[PATCH v2 0/7] serial: liteuart: add IRQ support

[PATCH v2 0/7] serial: liteuart: add IRQ support

[Gabriel Somlo]

Add IRQ support to the LiteX LiteUART serial interface

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 (7):
  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: separate RX loop from poll timer
  serial: liteuart: add IRQ support

 drivers/tty/serial/liteuart.c | 123 ++++++++++++++++++++++------------
 1 file changed, 80 insertions(+), 43 deletions(-)

-- 
2.37.3

[PATCH v2 1/7] serial: liteuart: use KBUILD_MODNAME as driver name

[Gabriel Somlo]

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

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

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

[PATCH v2 2/7] serial: liteuart: use bit number macros

[Gabriel Somlo]

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

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

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

[PATCH v2 3/7] serial: liteuart: remove unused uart_ops stubs

[Gabriel Somlo]

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

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

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

[PATCH v2 4/7] serial: liteuart: don't set unused port fields

[Gabriel Somlo]

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

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

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

[PATCH v2 5/7] serial: liteuart: minor style fix in liteuart_init()

[Gabriel Somlo]

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

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

[PATCH v2 6/7] serial: liteuart: separate RX loop from poll timer

[Gabriel Somlo]

Move the character-receive (RX) loop to its own dedicated function,
and (for now) call that from the poll timer, liteuart_timer().

This is in preparation for adding IRQ support to the receive path.

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

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 047d5ad32e13..aa7052280197 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -67,29 +67,34 @@ 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 flg = TTY_NORMAL;
-	int ch;
-	unsigned long status;
+	unsigned int status;
+	unsigned char ch;
 
 	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
 		ch = litex_read8(membase + OFF_RXTX);
 		port->icount.rx++;
 
 		/* necessary for RXEMPTY to refresh its value */
-		litex_write8(membase + OFF_EV_PENDING, EV_TX | EV_RX);
+		litex_write8(membase + OFF_EV_PENDING, EV_RX);
 
 		/* no overflow bits in status */
 		if (!(uart_handle_sysrq_char(port, ch)))
-			uart_insert_char(port, status, 0, ch, flg);
-
-		tty_flip_buffer_push(&port->state->port);
+			uart_insert_char(port, status, 0, ch, TTY_NORMAL);
 	}
 
+	tty_flip_buffer_push(&port->state->port);
+}
+
+static void liteuart_timer(struct timer_list *t)
+{
+	struct liteuart_port *uart = from_timer(uart, t, timer);
+	struct uart_port *port = &uart->port;
+
+	liteuart_rx_chars(port);
+
 	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
 }
 
-- 
2.37.3

[PATCH v2 7/7] serial: liteuart: add IRQ support

[Gabriel Somlo]

Add support for IRQ-driven RX. The TX path remains "polling" based,
which is fine since TX is synchronous.

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

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index aa7052280197..45da944d1fea 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/console.h>
+#include <linux/interrupt.h>
 #include <linux/litex.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -88,13 +89,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 uart_port *port = data;
+	unsigned int isr;
+
+	isr = litex_read32(port->membase + OFF_EV_PENDING);
+
+	spin_lock(&port->lock);
+	if (isr & EV_RX)
+		liteuart_rx_chars(port);
+	spin_unlock(&port->lock);
+
+	return IRQ_RETVAL(isr);
+}
+
 static void liteuart_timer(struct timer_list *t)
 {
 	struct liteuart_port *uart = from_timer(uart, t, timer);
 	struct uart_port *port = &uart->port;
 
-	liteuart_rx_chars(port);
-
+	liteuart_interrupt(0, port);
 	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
 }
 
@@ -163,19 +178,49 @@ static void liteuart_stop_rx(struct uart_port *port)
 static int liteuart_startup(struct uart_port *port)
 {
 	struct liteuart_port *uart = to_liteuart_port(port);
+	unsigned long flags;
+	int ret;
+	u8 irq_mask = 0;
 
-	/* disable events */
-	litex_write8(port->membase + OFF_EV_ENABLE, 0);
+	if (port->irq) {
+		ret = request_irq(port->irq, liteuart_interrupt, 0,
+				  KBUILD_MODNAME, port);
+		if (ret == 0) {
+			/* we only need interrupts on the rx path! */
+			irq_mask = EV_RX;
+		} else {
+			pr_err(KBUILD_MODNAME ": can't attach LiteUART %d "
+				"irq %d; switching 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));
+	if (!port->irq) {
+		timer_setup(&uart->timer, liteuart_timer, 0);
+		mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
+	}
+
+	spin_lock_irqsave(&port->lock, flags);
+	litex_write8(port->membase + OFF_EV_ENABLE, irq_mask);
+	spin_unlock_irqrestore(&port->lock, flags);
 
 	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);
+	litex_write8(port->membase + OFF_EV_ENABLE, 0);
+	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,
@@ -264,6 +309,13 @@ static int liteuart_probe(struct platform_device *pdev)
 		goto err_erase_id;
 	}
 
+	/* get irq */
+	ret = platform_get_irq_optional(pdev, 0);
+	if (ret < 0 && ret != -ENXIO)
+		return ret;
+	if (ret > 0)
+		port->irq = ret;
+
 	/* values not from device tree */
 	port->dev = &pdev->dev;
 	port->iotype = UPIO_MEM;
-- 
2.37.3

Re: [PATCH v2 6/7] serial: liteuart: separate RX loop from poll timer

[Ilpo Järvinen]

On Wed, 9 Nov 2022, Gabriel Somlo wrote:

> Move the character-receive (RX) loop to its own dedicated function,
> and (for now) call that from the poll timer, liteuart_timer().
> 
> This is in preparation for adding IRQ support to the receive path.
> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 047d5ad32e13..aa7052280197 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -67,29 +67,34 @@ 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 flg = TTY_NORMAL;
> -	int ch;
> -	unsigned long status;
> +	unsigned int status;
> +	unsigned char ch;
>  
>  	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
>  		ch = litex_read8(membase + OFF_RXTX);
>  		port->icount.rx++;
>  
>  		/* necessary for RXEMPTY to refresh its value */
> -		litex_write8(membase + OFF_EV_PENDING, EV_TX | EV_RX);
> +		litex_write8(membase + OFF_EV_PENDING, EV_RX);
>  
>  		/* no overflow bits in status */
>  		if (!(uart_handle_sysrq_char(port, ch)))
> -			uart_insert_char(port, status, 0, ch, flg);
> -
> -		tty_flip_buffer_push(&port->state->port);
> +			uart_insert_char(port, status, 0, ch, TTY_NORMAL);
>  	}
>  
> +	tty_flip_buffer_push(&port->state->port);

This change is doing extra stuff besides moving rx to a dedicated 
function.

I see no reason why those other changes couldn't be put into an entirely 
separate patch. Also, please described those changes properly in the 
commit message (answer the why? question).

-- 
 i.

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

Re: [PATCH v2 6/7] serial: liteuart: separate RX loop from poll timer

[Gabriel L. Somlo]

On Thu, Nov 10, 2022 at 01:01:47PM +0200, Ilpo Järvinen wrote:
> On Wed, 9 Nov 2022, Gabriel Somlo wrote:
> 
> > Move the character-receive (RX) loop to its own dedicated function,
> > and (for now) call that from the poll timer, liteuart_timer().
> > 
> > This is in preparation for adding IRQ support to the receive path.
> > 
> > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> > ---
> >  drivers/tty/serial/liteuart.c | 25 +++++++++++++++----------
> >  1 file changed, 15 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > index 047d5ad32e13..aa7052280197 100644
> > --- a/drivers/tty/serial/liteuart.c
> > +++ b/drivers/tty/serial/liteuart.c
> > @@ -67,29 +67,34 @@ 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 flg = TTY_NORMAL;
> > -	int ch;
> > -	unsigned long status;
> > +	unsigned int status;
> > +	unsigned char ch;
> >  
> >  	while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
> >  		ch = litex_read8(membase + OFF_RXTX);
> >  		port->icount.rx++;
> >  
> >  		/* necessary for RXEMPTY to refresh its value */
> > -		litex_write8(membase + OFF_EV_PENDING, EV_TX | EV_RX);
> > +		litex_write8(membase + OFF_EV_PENDING, EV_RX);
> >  
> >  		/* no overflow bits in status */
> >  		if (!(uart_handle_sysrq_char(port, ch)))
> > -			uart_insert_char(port, status, 0, ch, flg);
> > -
> > -		tty_flip_buffer_push(&port->state->port);
> > +			uart_insert_char(port, status, 0, ch, TTY_NORMAL);
> >  	}
> >  
> > +	tty_flip_buffer_push(&port->state->port);
> 
> This change is doing extra stuff besides moving rx to a dedicated 
> function.
> 
> I see no reason why those other changes couldn't be put into an entirely 
> separate patch. Also, please described those changes properly in the 
> commit message (answer the why? question).
 
You're right, calling `tty_flip_buffer_push()` as each character is
received is overkill, we only need to call it once per interrupt once
all available characters have been received.

I forgot I noticed (and fixed) that as part of the move -- I'll split
it out into its own separate patch (probably *before* moving all of rx
to a dedicated function).

Should show up in v3, once I also address all the other feedback I
received.

Thanks again for catching it!

Cheers,
-Gabriel

> -- 
>  i.
> 
> > +}
> > +
> > +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));
> >  }
> >  
> > 
> 
>