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

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

[Gabriel Somlo]

Add IRQ support to the LiteX LiteUART serial interface

Changes from v3:
  - picked up r/b Ilpo Järvinen on select subset of commits
  - rebased entire series on top of tty-next tree
  - 2/14 ("use bit number macros"): explicitly include <linux/bits.h>
  - 3/14 ("remove unused uart_ops stubs"): don't add gratuitous comment
    removed later on in the series
  - 12/14 ("add IRQ support for the RX path"): add shadow irq register
    to support polling mode and avoid reading hardware mmio irq register
    to learn which irq flags are enabled
    - this also simplifies both liteuart_interrupt() and liteuart_startup()
  - 13/14 ("add IRQ support for the TX path"):
    - removed superfluous curly braces from liteuart_interrupt()
    - simplified [start|stop]_tx() by using shadow irq register from 12/14
    - simplified liteuart_tx_chars() by rebasing on top of tty-next tree

> 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 | 210 ++++++++++++++++++++--------------
 1 file changed, 126 insertions(+), 84 deletions(-)

-- 
2.38.1

[PATCH v4 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 062812fe1b09..db898751ffe3 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,
@@ -321,7 +321,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,
 	},
 };
@@ -367,7 +367,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.38.1

[PATCH v4 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>
---

New in v4:
  - explicitly include <linux/bits.h>

 drivers/tty/serial/liteuart.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index db898751ffe3..18c1eb315ee9 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2019-2020 Antmicro <www.antmicro.com>
  */
 
+#include <linux/bits.h>
 #include <linux/console.h>
 #include <linux/litex.h>
 #include <linux/module.h>
@@ -38,8 +39,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.38.1

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

[Gabriel Somlo]

Remove stub uart_ops methods that are not called unconditionally
from serial_core.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

Changes since v3:
  - no longer adding gratuitous comment to liteuart_stop_tx,
    removed later on in the series

 drivers/tty/serial/liteuart.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 18c1eb315ee9..989a4f8d5bd4 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -154,11 +154,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 +192,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 +218,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.38.1

[PATCH v4 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 989a4f8d5bd4..c6eb7eba5af8 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -263,9 +263,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.38.1

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

[Gabriel Somlo]

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.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 c6eb7eba5af8..1e3429bcc2ad 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -397,12 +397,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.38.1

[PATCH v4 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.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 1e3429bcc2ad..81a86c5eb393 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -87,10 +87,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.38.1

[PATCH v4 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 81a86c5eb393..c90ab65fbdcf 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -82,7 +82,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.38.1

[PATCH v4 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.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 c90ab65fbdcf..81aa7c1da73c 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -73,7 +73,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;
 
@@ -86,7 +85,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.38.1

[PATCH v4 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.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 81aa7c1da73c..42ac9aee050a 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -73,8 +73,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.38.1

[PATCH v4 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.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 42ac9aee050a..76f8a09b82cd 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -68,10 +68,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;
 
@@ -88,6 +86,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.38.1

[PATCH v4 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/liteuart.c | 82 +++++++++++++++++------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 76f8a09b82cd..8a6e176be08e 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -68,6 +68,47 @@ 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)
+{
+}
+
+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];
+			uart_xmit_advance(port, 1);
+			liteuart_putchar(port, ch);
+		}
+	}
+
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+		uart_write_wakeup(port);
+}
+
+static void liteuart_stop_rx(struct uart_port *port)
+{
+	struct liteuart_port *uart = to_liteuart_port(port);
+
+	/* just delete timer */
+	del_timer(&uart->timer);
+}
+
 static void liteuart_rx_chars(struct uart_port *port)
 {
 	unsigned char __iomem *membase = port->membase;
@@ -98,14 +139,6 @@ static void liteuart_timer(struct timer_list *t)
 	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))
-		cpu_relax();
-
-	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 */
@@ -125,39 +158,6 @@ 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)
-{
-}
-
-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];
-			uart_xmit_advance(port, 1);
-			liteuart_putchar(port, ch);
-		}
-	}
-
-	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
-		uart_write_wakeup(port);
-}
-
-static void liteuart_stop_rx(struct uart_port *port)
-{
-	struct liteuart_port *uart = to_liteuart_port(port);
-
-	/* just delete timer */
-	del_timer(&uart->timer);
-}
-
 static int liteuart_startup(struct uart_port *port)
 {
 	struct liteuart_port *uart = to_liteuart_port(port);
-- 
2.38.1

[PATCH v4 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>
---

Changes from v3:
  - add shadow irq register to support polling mode and avoid reading
    hardware mmio irq register to learn which irq flags are enabled
    - this also simplifies both liteuart_interrupt() and liteuart_startup()

 drivers/tty/serial/liteuart.c | 76 +++++++++++++++++++++++++++++++----
 1 file changed, 69 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 8a6e176be08e..fad778578986 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -7,6 +7,7 @@
 
 #include <linux/bits.h>
 #include <linux/console.h>
+#include <linux/interrupt.h>
 #include <linux/litex.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -46,6 +47,7 @@ struct liteuart_port {
 	struct uart_port port;
 	struct timer_list timer;
 	u32 id;
+	u8 irq_reg;
 };
 
 #define to_liteuart_port(port)	container_of(port, struct liteuart_port, port)
@@ -76,6 +78,19 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
 	litex_write8(port->membase + OFF_RXTX, ch);
 }
 
+static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 mask)
+{
+	struct liteuart_port *uart = to_liteuart_port(port);
+
+	if (set)
+		uart->irq_reg |= mask;
+	else
+		uart->irq_reg &= ~mask;
+
+	if (port->irq)
+		litex_write8(port->membase + OFF_EV_ENABLE, uart->irq_reg);
+}
+
 static void liteuart_stop_tx(struct uart_port *port)
 {
 }
@@ -129,13 +144,27 @@ 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;
+
+	spin_lock(&port->lock);
+	isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
+	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));
 }
 
@@ -161,19 +190,45 @@ 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;
 
-	/* 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) {
+			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));
+	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) {
+		timer_setup(&uart->timer, liteuart_timer, 0);
+		mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
+	}
 
 	return 0;
 }
 
 static void liteuart_shutdown(struct uart_port *port)
 {
+	struct liteuart_port *uart = to_liteuart_port(port);
+	unsigned long flags;
+
+	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);
+	else
+		del_timer_sync(&uart->timer);
 }
 
 static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
@@ -262,6 +317,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.38.1

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

[Gabriel Somlo]

Switch the TX path to IRQ-driven operation, while maintaining support
for polling mode via the poll timer.

Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---

Changes from v3:
  - remove superfluous curly braces from liteuart_interrupt()
  - simplified [start|stop]_tx() by using shadow irq register from
    patch 12/14
  - simplified liteuart_tx_chars() by rebasing on top of tty-next tree

 drivers/tty/serial/liteuart.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index fad778578986..977fc4349b47 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -93,27 +93,12 @@ static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 mask)
 
 static void liteuart_stop_tx(struct uart_port *port)
 {
+	liteuart_update_irq_reg(port, false, EV_TX);
 }
 
 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];
-			uart_xmit_advance(port, 1);
-			liteuart_putchar(port, ch);
-		}
-	}
-
-	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
-		uart_write_wakeup(port);
+	liteuart_update_irq_reg(port, true, EV_TX);
 }
 
 static void liteuart_stop_rx(struct uart_port *port)
@@ -144,6 +129,15 @@ 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)
+{
+	u8 ch;
+
+	uart_port_tx(port, ch,
+		!litex_read8(port->membase + OFF_TXFULL),
+		litex_write8(port->membase + OFF_RXTX, ch));
+}
+
 static irqreturn_t liteuart_interrupt(int irq, void *data)
 {
 	struct liteuart_port *uart = data;
@@ -154,6 +148,8 @@ static irqreturn_t liteuart_interrupt(int irq, void *data)
 	isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
 	if (isr & EV_RX)
 		liteuart_rx_chars(port);
+	if (isr & EV_TX)
+		liteuart_tx_chars(port);
 	spin_unlock(&port->lock);
 
 	return IRQ_RETVAL(isr);
-- 
2.38.1

[PATCH v4 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.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 977fc4349b47..238f615cb80e 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -70,14 +70,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_update_irq_reg(struct uart_port *port, bool set, u8 mask)
 {
 	struct liteuart_port *uart = to_liteuart_port(port);
@@ -372,6 +364,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.38.1

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

[Ilpo Järvinen]

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

On Wed, 16 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>
> ---
> 
> Changes from v3:
>   - add shadow irq register to support polling mode and avoid reading
>     hardware mmio irq register to learn which irq flags are enabled
>     - this also simplifies both liteuart_interrupt() and liteuart_startup()
> 
>  drivers/tty/serial/liteuart.c | 76 +++++++++++++++++++++++++++++++----
>  1 file changed, 69 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 8a6e176be08e..fad778578986 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/bits.h>
>  #include <linux/console.h>
> +#include <linux/interrupt.h>
>  #include <linux/litex.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> @@ -46,6 +47,7 @@ struct liteuart_port {
>  	struct uart_port port;
>  	struct timer_list timer;
>  	u32 id;
> +	u8 irq_reg;
>  };
>  
>  #define to_liteuart_port(port)	container_of(port, struct liteuart_port, port)
> @@ -76,6 +78,19 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
>  	litex_write8(port->membase + OFF_RXTX, ch);
>  }
>  
> +static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 mask)
> +{
> +	struct liteuart_port *uart = to_liteuart_port(port);
> +
> +	if (set)
> +		uart->irq_reg |= mask;
> +	else
> +		uart->irq_reg &= ~mask;
> +
> +	if (port->irq)
> +		litex_write8(port->membase + OFF_EV_ENABLE, uart->irq_reg);
> +}
> +
>  static void liteuart_stop_tx(struct uart_port *port)
>  {
>  }
> @@ -129,13 +144,27 @@ 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;
> +
> +	spin_lock(&port->lock);
> +	isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
> +	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));
>  }
>  
> @@ -161,19 +190,45 @@ 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;
>  
> -	/* 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) {
> +			pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
> +				port->line, port->irq);

dev_err() seems more appropriate here.

> +			port->irq = 0;
> +		}
> +	}
>  
> -	/* prepare timer for polling */
> -	timer_setup(&uart->timer, liteuart_timer, 0);
> -	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> +	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) {
> +		timer_setup(&uart->timer, liteuart_timer, 0);
> +		mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> +	}
>  
>  	return 0;
>  }
>  
>  static void liteuart_shutdown(struct uart_port *port)
>  {
> +	struct liteuart_port *uart = to_liteuart_port(port);
> +	unsigned long flags;
> +
> +	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);
> +	else
> +		del_timer_sync(&uart->timer);
>  }
>  
>  static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
> @@ -262,6 +317,13 @@ static int liteuart_probe(struct platform_device *pdev)
>  		goto err_erase_id;
>  	}
>  
> +	/* get irq */

This comment is unnecessary.

Other than that,

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

-- 
 i.


> +	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 v4 13/14] serial: liteuart: add IRQ support for the TX path

[Ilpo Järvinen]

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

On Wed, 16 Nov 2022, Gabriel Somlo wrote:

> Switch the TX path to IRQ-driven operation, while maintaining support
> for polling mode via the poll timer.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
> 
> Changes from v3:
>   - remove superfluous curly braces from liteuart_interrupt()
>   - simplified [start|stop]_tx() by using shadow irq register from
>     patch 12/14
>   - simplified liteuart_tx_chars() by rebasing on top of tty-next tree
> 
>  drivers/tty/serial/liteuart.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index fad778578986..977fc4349b47 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -93,27 +93,12 @@ static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 mask)
>  
>  static void liteuart_stop_tx(struct uart_port *port)
>  {
> +	liteuart_update_irq_reg(port, false, EV_TX);
>  }
>  
>  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];
> -			uart_xmit_advance(port, 1);
> -			liteuart_putchar(port, ch);
> -		}
> -	}
> -
> -	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> -		uart_write_wakeup(port);
> +	liteuart_update_irq_reg(port, true, EV_TX);
>  }
>  
>  static void liteuart_stop_rx(struct uart_port *port)
> @@ -144,6 +129,15 @@ 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)
> +{
> +	u8 ch;
> +
> +	uart_port_tx(port, ch,
> +		!litex_read8(port->membase + OFF_TXFULL),
> +		litex_write8(port->membase + OFF_RXTX, ch));
> +}
> +
>  static irqreturn_t liteuart_interrupt(int irq, void *data)
>  {
>  	struct liteuart_port *uart = data;
> @@ -154,6 +148,8 @@ static irqreturn_t liteuart_interrupt(int irq, void *data)
>  	isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
>  	if (isr & EV_RX)
>  		liteuart_rx_chars(port);
> +	if (isr & EV_TX)
> +		liteuart_tx_chars(port);
>  	spin_unlock(&port->lock);
>  
>  	return IRQ_RETVAL(isr);

Looks very clean now with uart_port_tx(), thanks.

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

-- 
 i.

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

[Gabriel L. Somlo]

On Fri, Nov 18, 2022 at 03:58:47PM +0200, Ilpo Järvinen wrote:
> On Wed, 16 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>
> > ---
> > 
> > Changes from v3:
> >   - add shadow irq register to support polling mode and avoid reading
> >     hardware mmio irq register to learn which irq flags are enabled
> >     - this also simplifies both liteuart_interrupt() and liteuart_startup()
> > 
> >  drivers/tty/serial/liteuart.c | 76 +++++++++++++++++++++++++++++++----
> >  1 file changed, 69 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > index 8a6e176be08e..fad778578986 100644
> > --- a/drivers/tty/serial/liteuart.c
> > +++ b/drivers/tty/serial/liteuart.c
> > @@ -7,6 +7,7 @@
> >  
> >  #include <linux/bits.h>
> >  #include <linux/console.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/litex.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > @@ -46,6 +47,7 @@ struct liteuart_port {
> >  	struct uart_port port;
> >  	struct timer_list timer;
> >  	u32 id;
> > +	u8 irq_reg;
> >  };
> >  
> >  #define to_liteuart_port(port)	container_of(port, struct liteuart_port, port)
> > @@ -76,6 +78,19 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
> >  	litex_write8(port->membase + OFF_RXTX, ch);
> >  }
> >  
> > +static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 mask)
> > +{
> > +	struct liteuart_port *uart = to_liteuart_port(port);
> > +
> > +	if (set)
> > +		uart->irq_reg |= mask;
> > +	else
> > +		uart->irq_reg &= ~mask;
> > +
> > +	if (port->irq)
> > +		litex_write8(port->membase + OFF_EV_ENABLE, uart->irq_reg);
> > +}
> > +
> >  static void liteuart_stop_tx(struct uart_port *port)
> >  {
> >  }
> > @@ -129,13 +144,27 @@ 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;
> > +
> > +	spin_lock(&port->lock);
> > +	isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
> > +	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));
> >  }
> >  
> > @@ -161,19 +190,45 @@ 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;
> >  
> > -	/* 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) {
> > +			pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
> > +				port->line, port->irq);
> 
> dev_err() seems more appropriate here.

Makes sense, good thing `struct uart_port` has a `dev` field :)
I've made the change, and lined it up for the upcoming v5 of the
series.
 
> > +			port->irq = 0;
> > +		}
> > +	}
> >  
> > -	/* prepare timer for polling */
> > -	timer_setup(&uart->timer, liteuart_timer, 0);
> > -	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > +	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) {
> > +		timer_setup(&uart->timer, liteuart_timer, 0);
> > +		mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > +	}
> >  
> >  	return 0;
> >  }
> >  
> >  static void liteuart_shutdown(struct uart_port *port)
> >  {
> > +	struct liteuart_port *uart = to_liteuart_port(port);
> > +	unsigned long flags;
> > +
> > +	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);
> > +	else
> > +		del_timer_sync(&uart->timer);
> >  }
> >  
> >  static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
> > @@ -262,6 +317,13 @@ static int liteuart_probe(struct platform_device *pdev)
> >  		goto err_erase_id;
> >  	}
> >  
> > +	/* get irq */
> 
> This comment is unnecessary.

Removed in v5.

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

Thanks much, I'll send out v5 a bit later today.

--Gabriel

> -- 
>  i.
> 
> 
> > +	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 v4 12/14] serial: liteuart: add IRQ support for the RX path

[Ilpo Järvinen]

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

On Fri, 18 Nov 2022, Gabriel L. Somlo wrote:

> On Fri, Nov 18, 2022 at 03:58:47PM +0200, Ilpo Järvinen wrote:
> > On Wed, 16 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>
> > > ---
> > > 
> > > Changes from v3:
> > >   - add shadow irq register to support polling mode and avoid reading
> > >     hardware mmio irq register to learn which irq flags are enabled
> > >     - this also simplifies both liteuart_interrupt() and liteuart_startup()
> > > 
> > >  drivers/tty/serial/liteuart.c | 76 +++++++++++++++++++++++++++++++----
> > >  1 file changed, 69 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > > index 8a6e176be08e..fad778578986 100644
> > > --- a/drivers/tty/serial/liteuart.c
> > > +++ b/drivers/tty/serial/liteuart.c
> > > @@ -7,6 +7,7 @@
> > >  
> > >  #include <linux/bits.h>
> > >  #include <linux/console.h>
> > > +#include <linux/interrupt.h>
> > >  #include <linux/litex.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > > @@ -46,6 +47,7 @@ struct liteuart_port {
> > >  	struct uart_port port;
> > >  	struct timer_list timer;
> > >  	u32 id;
> > > +	u8 irq_reg;
> > >  };
> > >  
> > >  #define to_liteuart_port(port)	container_of(port, struct liteuart_port, port)
> > > @@ -76,6 +78,19 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
> > >  	litex_write8(port->membase + OFF_RXTX, ch);
> > >  }
> > >  
> > > +static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 mask)
> > > +{
> > > +	struct liteuart_port *uart = to_liteuart_port(port);
> > > +
> > > +	if (set)
> > > +		uart->irq_reg |= mask;
> > > +	else
> > > +		uart->irq_reg &= ~mask;
> > > +
> > > +	if (port->irq)
> > > +		litex_write8(port->membase + OFF_EV_ENABLE, uart->irq_reg);
> > > +}
> > > +
> > >  static void liteuart_stop_tx(struct uart_port *port)
> > >  {
> > >  }
> > > @@ -129,13 +144,27 @@ 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;
> > > +
> > > +	spin_lock(&port->lock);
> > > +	isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
> > > +	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));
> > >  }
> > >  
> > > @@ -161,19 +190,45 @@ 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;
> > >  
> > > -	/* 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) {
> > > +			pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
> > > +				port->line, port->irq);
> > 
> > dev_err() seems more appropriate here.
> 
> Makes sense, good thing `struct uart_port` has a `dev` field :)
> I've made the change, and lined it up for the upcoming v5 of the
> series.

Also that pr_fmt() should be dropped (in case you didn't already). It was 
in my mind ealier but should have stated it explicitly.


-- 
 i.

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

[Gabriel L. Somlo]

On Fri, Nov 18, 2022 at 04:30:15PM +0200, Ilpo Järvinen wrote:
> On Fri, 18 Nov 2022, Gabriel L. Somlo wrote:
> 
> > On Fri, Nov 18, 2022 at 03:58:47PM +0200, Ilpo Järvinen wrote:
> > > On Wed, 16 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>
> > > > ---
> > > > 
> > > > Changes from v3:
> > > >   - add shadow irq register to support polling mode and avoid reading
> > > >     hardware mmio irq register to learn which irq flags are enabled
> > > >     - this also simplifies both liteuart_interrupt() and liteuart_startup()
> > > > 
> > > >  drivers/tty/serial/liteuart.c | 76 +++++++++++++++++++++++++++++++----
> > > >  1 file changed, 69 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > > > index 8a6e176be08e..fad778578986 100644
> > > > --- a/drivers/tty/serial/liteuart.c
> > > > +++ b/drivers/tty/serial/liteuart.c
> > > > @@ -7,6 +7,7 @@
> > > >  
> > > >  #include <linux/bits.h>
> > > >  #include <linux/console.h>
> > > > +#include <linux/interrupt.h>
> > > >  #include <linux/litex.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > > @@ -46,6 +47,7 @@ struct liteuart_port {
> > > >  	struct uart_port port;
> > > >  	struct timer_list timer;
> > > >  	u32 id;
> > > > +	u8 irq_reg;
> > > >  };
> > > >  
> > > >  #define to_liteuart_port(port)	container_of(port, struct liteuart_port, port)
> > > > @@ -76,6 +78,19 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
> > > >  	litex_write8(port->membase + OFF_RXTX, ch);
> > > >  }
> > > >  
> > > > +static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 mask)
> > > > +{
> > > > +	struct liteuart_port *uart = to_liteuart_port(port);
> > > > +
> > > > +	if (set)
> > > > +		uart->irq_reg |= mask;
> > > > +	else
> > > > +		uart->irq_reg &= ~mask;
> > > > +
> > > > +	if (port->irq)
> > > > +		litex_write8(port->membase + OFF_EV_ENABLE, uart->irq_reg);
> > > > +}
> > > > +
> > > >  static void liteuart_stop_tx(struct uart_port *port)
> > > >  {
> > > >  }
> > > > @@ -129,13 +144,27 @@ 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;
> > > > +
> > > > +	spin_lock(&port->lock);
> > > > +	isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
> > > > +	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));
> > > >  }
> > > >  
> > > > @@ -161,19 +190,45 @@ 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;
> > > >  
> > > > -	/* 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) {
> > > > +			pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
> > > > +				port->line, port->irq);
> > > 
> > > dev_err() seems more appropriate here.
> > 
> > Makes sense, good thing `struct uart_port` has a `dev` field :)
> > I've made the change, and lined it up for the upcoming v5 of the
> > series.
> 
> Also that pr_fmt() should be dropped (in case you didn't already). It was 
> in my mind ealier but should have stated it explicitly.

Understood. I've used dev_err() before, it just slipped my mind to
check for the availability of a `dev` field in `struct uart_port`...  :)

Thanks again for the review!

Best,
--Gabriel