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

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

[Gabriel Somlo]

Add IRQ support to the LiteX LiteUART serial interface

Changes from v4:
  - picked up r/b Ilpo Järvinen on 12/14 and 13/14
  - 12/14 ("add IRQ support for the RX path"): using dev_err() instead
    of combining pr_err() and pr_fmt(); also, remove superfluous comment

> 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 v5 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 v5 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 | 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 v5 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>
---
 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 v5 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 v5 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 v5 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 v5 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 v5 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 v5 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 v5 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 v5 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 v5 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

Changes from v4:
  - using dev_err() instead of a combo of pr_err() and pr_fmt()
  - dropped "get irq" comment in probe()

> 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..678c37c952cf 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,46 @@ 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) {
+			dev_err(port->dev,
+				"line %d irq %d failed: switch to 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 +318,12 @@ static int liteuart_probe(struct platform_device *pdev)
 		goto err_erase_id;
 	}
 
+	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 v5 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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 678c37c952cf..850125870efb 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 v5 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 850125870efb..47d12bb1b9e0 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 v5 09/14] serial: liteuart: fix rx loop variable types

[Jiri Slaby]

On 18. 11. 22, 15:55, 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>
> 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;

These should be u8 after all, right?

Wait, status is a bool in the end:

>   	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {

But why is it passed to uart_insert_char() as such? That's ugly. Maybe 
drop all of this "status" and pass LSR_RXC directly. The while's 
condition would simply look like (!litex_read8(membase + OFF_RXEMPTY)) then.

>   		ch = litex_read8(membase + OFF_RXTX);

thanks,
-- 
js
suse labs

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

[Jiri Slaby]

On 21. 11. 22, 9:37, Jiri Slaby wrote:
> On 18. 11. 22, 15:55, 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>
>> 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;
> 
> These should be u8 after all, right?
> 
> Wait, status is a bool in the end:
> 
>>       while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
> 
> But why is it passed to uart_insert_char() as such? That's ugly. Maybe 
> drop all of this "status" and pass LSR_RXC directly.

Actually, even 0 directly, provided overrun parameter is always 0.

> The while's 
> condition would simply look like (!litex_read8(membase + OFF_RXEMPTY)) 
> then.
> 
>>           ch = litex_read8(membase + OFF_RXTX);
> 
> thanks,

-- 
js

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

[Jiri Slaby]

On 21. 11. 22, 9:37, Jiri Slaby wrote:
> On 18. 11. 22, 15:55, 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>
>> 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;
> 
> These should be u8 after all, right?

And can you change membase to u8 * too 8-)?

-- 
js
suse labs

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

[Jiri Slaby]

On 18. 11. 22, 15:55, 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>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 
> Changes from v4:
>    - using dev_err() instead of a combo of pr_err() and pr_fmt()
>    - dropped "get irq" comment in probe()
> 
>> 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..678c37c952cf 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);

Are you sure spin_lock() is safe from this path? I assume so, but have 
you thought about it?

>   	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
>   }
>   
> @@ -161,19 +190,46 @@ 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);

Just asking: cannot the irq be shared?

> +		if (ret) {
> +			dev_err(port->dev,
> +				"line %d irq %d failed: switch to polling\n",
> +				port->line, port->irq);

That is, it should be only dev_warn(), or?

> +			port->irq = 0;
> +		}
> +	}

thanks,
-- 
js
suse labs

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

[Jiri Slaby]

On 18. 11. 22, 15:55, 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>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
...
> @@ -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);

Wait, how do you ensure the OFF_EV_PENDING reg contains EV_RX and/or 
EV_TX in the polling mode?

thanks,
-- 
js
suse labs

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

[Geert Uytterhoeven]

On Fri, Nov 18, 2022 at 3:56 PM Gabriel Somlo <gsomlo@gmail.com> wrote:
> Replace hard-coded instances of "liteuart" with KBUILD_MODNAME.
>
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

On Fri, Nov 18, 2022 at 3:55 PM Gabriel Somlo <gsomlo@gmail.com> wrote:
> Replace magic bit constants (e.g., 1, 2, 4) with BIT(x) expressions.
>
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

On Fri, Nov 18, 2022 at 3:55 PM Gabriel Somlo <gsomlo@gmail.com> wrote:
> 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>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

On Fri, Nov 18, 2022 at 3:55 PM Gabriel Somlo <gsomlo@gmail.com> wrote:
> Remove regshift and iobase port fields, since they are unused
> by the driver.
>
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

On Fri, Nov 18, 2022 at 3:56 PM Gabriel Somlo <gsomlo@gmail.com> wrote:
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

On Fri, Nov 18, 2022 at 3:56 PM Gabriel Somlo <gsomlo@gmail.com> 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>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

On Fri, Nov 18, 2022 at 3:56 PM Gabriel Somlo <gsomlo@gmail.com> 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>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

Hi Gabriel,

On Fri, Nov 18, 2022 at 3:57 PM Gabriel Somlo <gsomlo@gmail.com> 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>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

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

So first you spin this off into a separate function, so it can be
called from both the interrupt and polling paths.
Later, in "[PATCH v5 12/14] serial: liteuart: add IRQ support for the
RX path", you remove the call from the polling path...


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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

On Fri, Nov 18, 2022 at 3:57 PM Gabriel Somlo <gsomlo@gmail.com> 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>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Gabriel L. Somlo]

Hi Jiri,

Thanks for the feedback!

On Mon, Nov 21, 2022 at 09:45:05AM +0100, Jiri Slaby wrote:
> On 21. 11. 22, 9:37, Jiri Slaby wrote:
> > On 18. 11. 22, 15:55, 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>
> > > 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;
> > 
> > These should be u8 after all, right?

OK, so:

  - I can hard-code `status` as `1`, like so:

	while(!litex_read8(membase + OFF_RXEMPTY) {
		...
		if (!(uart_handle_sysrq_char(port, ch)))
			uart_insert_char(port, 1, 0, ch, TTY_NORMAL);

    ... since `status` would always be `1` inside the loop. So I'm
    basically going to get rid of it altogether.

  - `ch` is indeed *produced* by `litex_read8()`, which would make it
    `u8`. It is subsequently *consumed* by `uart_handle_sysrq_char()`
    and `uart_insert_char()`, which both expect `unsigned int`.

    If you think it's better to go with the type when the value is
    produced (as opposed to when it's consumed), I'm OK with that for
    the upcoming v6 of the series... :)
 
> And can you change membase to u8 * too 8-)?

Hmmm, in `struct uart_port` (in include/linux/serial_core.h), the
`membase` field is declared as:

	unsigned char __iomem   *membase;

which is why I'm thinking we should leave it as-is? Unless there are
plans (or a pending patch I'm unaware of) to switch the field in
include/linux/serial_core.h to `u8` as well? -- Please advise.

Thanks again,
--Gabriel

> -- 
> js
> suse labs
> 

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

[Gabriel L. Somlo]

On Mon, Nov 21, 2022 at 09:58:41AM +0100, Jiri Slaby wrote:
> On 18. 11. 22, 15:55, 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>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ...
> > @@ -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);
> 
> Wait, how do you ensure the OFF_EV_PENDING reg contains EV_RX and/or EV_TX
> in the polling mode?

The hardware (well, *gateware*) is coded to populate the EV_PENDING
register regardless of whether IRQs are enabled via the EV_ENABLE
register (or indeed, whether IRQs are even "wired into" the design at
all). See

https://github.com/enjoy-digital/litex/blob/master/litex/soc/cores/uart.py
and
https://github.com/enjoy-digital/litex/blob/master/litex/soc/interconnect/csr_eventmanager.py

for a starting point on why it's OK to just assume that...

Thanks,
--Gabriel

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

[Gabriel L. Somlo]

On Mon, Nov 21, 2022 at 11:16:14AM +0100, Geert Uytterhoeven wrote:
> Hi Gabriel,
> 
> On Fri, Nov 18, 2022 at 3:57 PM Gabriel Somlo <gsomlo@gmail.com> 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>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Thanks for your patch!
> 
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> > --- 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)
> 
> So first you spin this off into a separate function, so it can be
> called from both the interrupt and polling paths.
> Later, in "[PATCH v5 12/14] serial: liteuart: add IRQ support for the
> RX path", you remove the call from the polling path...
 
That's right -- the polling path now calls the IRQ handler,
which, in turn, calls `liteuart_rx_chars()` (and later `*_tx_chars()`
as well).

The polling timer becomes just an alternative way to invoke the irq
handler, giving it a chance to take care of any pending transfers in
either/both directions.

Thanks,
--Gabriel
 
> >  {
> > -       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));
> >  }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

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

[Gabriel L. Somlo]

Hi Jiri,

On Mon, Nov 21, 2022 at 09:54:34AM +0100, Jiri Slaby wrote:
> On 18. 11. 22, 15:55, 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>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> > 
> > Changes from v4:
> >    - using dev_err() instead of a combo of pr_err() and pr_fmt()
> >    - dropped "get irq" comment in probe()
> > 
> > > 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..678c37c952cf 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);
> 
> Are you sure spin_lock() is safe from this path? I assume so, but have you
> thought about it?

I checked and at that point `in_serving_softirq()` is true.

*However*, after studying spin_lock() and friends for a while, I'm
not quite clear on whether that unequivocally translates
to "yes, we're safe" :)

As such, I'm inclined to switch to `spin_lock_irqsave()` and
`spin_unlock_irqrestore()` even in the interrupt handler, which is
explicitly stated to be "safe from any context":
https://www.kernel.org/doc/html/v4.15/kernel-hacking/locking.html#cheat-sheet-for-locking

The alternative could be to set `TIMER_IRQSAFE` in `timer_setup()`,
but no other tty driver seems to be doing that, so I'd be a bit off
the beaten path there... :)

Please do let me know what you think about this, particularly if you
consider going the spin_lock_irqsave-everywhere-just-to-be-safe route 
overkill... :)
 
> >   	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> >   }
> > @@ -161,19 +190,46 @@ 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);
> 
> Just asking: cannot the irq be shared?

Given the way LiteX gateware is currently generated, each
irq-triggering device is given its own separate line. I don't think
setting the IRQF_SHARED flag actually *hurts* anything (no difference
in behavior while testing), but I don't think it's needed ATM.

> > +		if (ret) {
> > +			dev_err(port->dev,
> > +				"line %d irq %d failed: switch to polling\n",
> > +				port->line, port->irq);
> 
> That is, it should be only dev_warn(), or?

Makes sense, will use dev_warn() in v6.

Please LMK what you think about spin_lock[_irqsave] (and IRQF_SHARED),
and I'll send out v6 with all the necessary chances right after that.

Thanks much,
--Gabriel

> > +			port->irq = 0;
> > +		}
> > +	}
> 
> thanks,
> -- 
> js
> suse labs
> 

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

[Jiri Slaby]

On 21. 11. 22, 14:55, Gabriel L. Somlo wrote:
> Hi Jiri,
> 
> Thanks for the feedback!
> 
> On Mon, Nov 21, 2022 at 09:45:05AM +0100, Jiri Slaby wrote:
>> On 21. 11. 22, 9:37, Jiri Slaby wrote:
>>> On 18. 11. 22, 15:55, 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>
>>>> 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;
>>>
>>> These should be u8 after all, right?
> 
> OK, so:
> 
>    - I can hard-code `status` as `1`, like so:
> 
> 	while(!litex_read8(membase + OFF_RXEMPTY) {
> 		...
> 		if (!(uart_handle_sysrq_char(port, ch)))
> 			uart_insert_char(port, 1, 0, ch, TTY_NORMAL);
> 
>      ... since `status` would always be `1` inside the loop. So I'm
>      basically going to get rid of it altogether.

Yes, I had that in my mind. Except passing 1 to uart_insert_char() when 
overflow is hardwired to 0 makes no sense IMO :).

>    - `ch` is indeed *produced* by `litex_read8()`, which would make it
>      `u8`. It is subsequently *consumed* by `uart_handle_sysrq_char()`
>      and `uart_insert_char()`, which both expect `unsigned int`.

Ignore uart_handle_sysrq_char and uart_insert_char. They should be fixed 
one day. It should really be u8. All down the call chain (it magically 
turns into int in the sysrq handlers, then char is expected).

>      If you think it's better to go with the type when the value is
>      produced (as opposed to when it's consumed), I'm OK with that for
>      the upcoming v6 of the series... :)

Yes, please. We should slowly convert _all_ of them.

>> And can you change membase to u8 * too 8-)?
> 
> Hmmm, in `struct uart_port` (in include/linux/serial_core.h), the
> `membase` field is declared as:
> 
> 	unsigned char __iomem   *membase;
> 
> which is why I'm thinking we should leave it as-is? Unless there are
> plans (or a pending patch I'm unaware of) to switch the field in
> include/linux/serial_core.h to `u8` as well? -- Please advise.

Ah, then keep it. I somehow thought it's void *. And yes, even this 
should be u8 __iomem *, eventually.

thanks,
-- 
js
suse labs

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

[Jiri Slaby]

On 21. 11. 22, 19:50, Gabriel L. Somlo wrote:
>>>    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);
>>
>> Are you sure spin_lock() is safe from this path? I assume so, but have you
>> thought about it?
> 
> I checked and at that point `in_serving_softirq()` is true.
> 
> *However*, after studying spin_lock() and friends for a while, I'm
> not quite clear on whether that unequivocally translates
> to "yes, we're safe" :)

Depends whether some hard irq context is grabbing the port lock too. If 
it does, it will spin forever waiting for this soft irq to finish (never 
happens as it was interrupted).

> As such, I'm inclined to switch to `spin_lock_irqsave()` and
> `spin_unlock_irqrestore()` even in the interrupt handler, which is
> explicitly stated to be "safe from any context":
> https://www.kernel.org/doc/html/v4.15/kernel-hacking/locking.html#cheat-sheet-for-locking



> The alternative could be to set `TIMER_IRQSAFE` in `timer_setup()`,
> but no other tty driver seems to be doing that, so I'd be a bit off
> the beaten path there... :)

Ah, no.

> Please do let me know what you think about this, particularly if you
> consider going the spin_lock_irqsave-everywhere-just-to-be-safe route
> overkill... :)

If you are unsure about the other contexts, irqsave/restore is the way 
to go. It can be lifted later, if someone investigates harder.

thanks,
-- 
js

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

[Geert Uytterhoeven]

On Tue, Nov 22, 2022 at 8:44 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> On 21. 11. 22, 19:50, Gabriel L. Somlo wrote:
> >>>    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);
> >>
> >> Are you sure spin_lock() is safe from this path? I assume so, but have you
> >> thought about it?
> >
> > I checked and at that point `in_serving_softirq()` is true.
> >
> > *However*, after studying spin_lock() and friends for a while, I'm
> > not quite clear on whether that unequivocally translates
> > to "yes, we're safe" :)
>
> Depends whether some hard irq context is grabbing the port lock too. If
> it does, it will spin forever waiting for this soft irq to finish (never
> happens as it was interrupted).
>
> > As such, I'm inclined to switch to `spin_lock_irqsave()` and
> > `spin_unlock_irqrestore()` even in the interrupt handler, which is
> > explicitly stated to be "safe from any context":
> > https://www.kernel.org/doc/html/v4.15/kernel-hacking/locking.html#cheat-sheet-for-locking
>
>
>
> > The alternative could be to set `TIMER_IRQSAFE` in `timer_setup()`,
> > but no other tty driver seems to be doing that, so I'd be a bit off
> > the beaten path there... :)
>
> Ah, no.
>
> > Please do let me know what you think about this, particularly if you
> > consider going the spin_lock_irqsave-everywhere-just-to-be-safe route
> > overkill... :)
>
> If you are unsure about the other contexts, irqsave/restore is the way
> to go. It can be lifted later, if someone investigates harder.

Inside the interrupt handler, plain spin_lock() should be OK.
Inside the timer handler, I think you need to use spin_lock_irqsave(),
as e.g. liteuart_set_termios() (and lots of code in serial_core.c)
already uses spin_lock_irqsave().
Besides, on non-SMP, spin_lock() doesn't do anything, so you need
to rely on disabling the local interrupt.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Gabriel L. Somlo]

On Tue, Nov 22, 2022 at 10:54:45AM +0100, Geert Uytterhoeven wrote:
> On Tue, Nov 22, 2022 at 8:44 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> > On 21. 11. 22, 19:50, Gabriel L. Somlo wrote:
> > >>>    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);
> > >>
> > >> Are you sure spin_lock() is safe from this path? I assume so, but have you
> > >> thought about it?
> > >
> > > I checked and at that point `in_serving_softirq()` is true.
> > >
> > > *However*, after studying spin_lock() and friends for a while, I'm
> > > not quite clear on whether that unequivocally translates
> > > to "yes, we're safe" :)
> >
> > Depends whether some hard irq context is grabbing the port lock too. If
> > it does, it will spin forever waiting for this soft irq to finish (never
> > happens as it was interrupted).
> >
> > > As such, I'm inclined to switch to `spin_lock_irqsave()` and
> > > `spin_unlock_irqrestore()` even in the interrupt handler, which is
> > > explicitly stated to be "safe from any context":
> > > https://www.kernel.org/doc/html/v4.15/kernel-hacking/locking.html#cheat-sheet-for-locking
> >
> >
> >
> > > The alternative could be to set `TIMER_IRQSAFE` in `timer_setup()`,
> > > but no other tty driver seems to be doing that, so I'd be a bit off
> > > the beaten path there... :)
> >
> > Ah, no.
> >
> > > Please do let me know what you think about this, particularly if you
> > > consider going the spin_lock_irqsave-everywhere-just-to-be-safe route
> > > overkill... :)
> >
> > If you are unsure about the other contexts, irqsave/restore is the way
> > to go. It can be lifted later, if someone investigates harder.
> 
> Inside the interrupt handler, plain spin_lock() should be OK.
> Inside the timer handler, I think you need to use spin_lock_irqsave(),
> as e.g. liteuart_set_termios() (and lots of code in serial_core.c)
> already uses spin_lock_irqsave().
> Besides, on non-SMP, spin_lock() doesn't do anything, so you need
> to rely on disabling the local interrupt.

Thanks Geert for the clarification! I could write two wrappers around
the actual code doing the interrupt handler work, one with spin_lock()
for the actual irq handler and another with spin_lock_irqsave() for
the timer codepath. But to keep things simple I'm inclined to simply
use spin_lock_irqsave() and add a comment saying it's because we need
it when polling and it's not actually harmful when using IRQ.

Thanks,
--Gabriel
 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

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

[Gabriel L. Somlo]

On Tue, Nov 22, 2022 at 08:37:58AM +0100, Jiri Slaby wrote:
> On 21. 11. 22, 14:55, Gabriel L. Somlo wrote:
> > Hi Jiri,
> > 
> > Thanks for the feedback!
> > 
> > On Mon, Nov 21, 2022 at 09:45:05AM +0100, Jiri Slaby wrote:
> > > On 21. 11. 22, 9:37, Jiri Slaby wrote:
> > > > On 18. 11. 22, 15:55, 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>
> > > > > 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;
> > > > 
> > > > These should be u8 after all, right?
> > 
> > OK, so:
> > 
> >    - I can hard-code `status` as `1`, like so:
> > 
> > 	while(!litex_read8(membase + OFF_RXEMPTY) {
> > 		...
> > 		if (!(uart_handle_sysrq_char(port, ch)))
> > 			uart_insert_char(port, 1, 0, ch, TTY_NORMAL);
> > 
> >      ... since `status` would always be `1` inside the loop. So I'm
> >      basically going to get rid of it altogether.
> 
> Yes, I had that in my mind. Except passing 1 to uart_insert_char() when
> overflow is hardwired to 0 makes no sense IMO :).

So, looking at what uart_insert_char() does, I could simply do this
instead:

 	while(!litex_read8(membase + OFF_RXEMPTY) {
 		...
		/* LiteUART does not provide overrun bits */
 		if (!(uart_handle_sysrq_char(port, ch) ||
 		     tty_insert_flip_char(&port->state->port, ch, TTY_NORMAL)))
 			++port->icount.buf_overrun;
 
That is, `tty_insert_flip_char() is the portion of `uart_insert_char()`
that actually gets executed if status is 1 and overrun is 0...

I'm not quite confident about whether this is an improvement in legibility
and/or code quality, but please let me know what *you* think... :)

> >    - `ch` is indeed *produced* by `litex_read8()`, which would make it
> >      `u8`. It is subsequently *consumed* by `uart_handle_sysrq_char()`
> >      and `uart_insert_char()`, which both expect `unsigned int`.
> 
> Ignore uart_handle_sysrq_char and uart_insert_char. They should be fixed one
> day. It should really be u8. All down the call chain (it magically turns
> into int in the sysrq handlers, then char is expected).
>
> >      If you think it's better to go with the type when the value is
> >      produced (as opposed to when it's consumed), I'm OK with that for
> >      the upcoming v6 of the series... :)
> 
> Yes, please. We should slowly convert _all_ of them.

OK, u8 it is, then.
 
> > > And can you change membase to u8 * too 8-)?
> > 
> > Hmmm, in `struct uart_port` (in include/linux/serial_core.h), the
> > `membase` field is declared as:
> > 
> > 	unsigned char __iomem   *membase;
> > 
> > which is why I'm thinking we should leave it as-is? Unless there are
> > plans (or a pending patch I'm unaware of) to switch the field in
> > include/linux/serial_core.h to `u8` as well? -- Please advise.
> 
> Ah, then keep it. I somehow thought it's void *. And yes, even this should
> be u8 __iomem *, eventually.

OK, it should/will get updated in bulk once that change is made across
the entire subsystem, leaving as-is for now.

Thanks again for all your help and advice!
--Gabriel

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

[Jiri Slaby]

On 22. 11. 22, 22:05, Gabriel L. Somlo wrote:
> So, looking at what uart_insert_char() does, I could simply do this
> instead:
> 
>   	while(!litex_read8(membase + OFF_RXEMPTY) {
>   		...
> 		/* LiteUART does not provide overrun bits */
>   		if (!(uart_handle_sysrq_char(port, ch) ||
>   		     tty_insert_flip_char(&port->state->port, ch, TTY_NORMAL)))
>   			++port->icount.buf_overrun;
>   
> That is, `tty_insert_flip_char() is the portion of `uart_insert_char()`
> that actually gets executed if status is 1 and overrun is 0...
> 
> I'm not quite confident about whether this is an improvement in legibility
> and/or code quality,

It's not :). Keep the uart_ helper.

-- 
js
suse labs