[PATCH v1 0/3] serial: liteuart: add IRQ support

[PATCH v1 0/3] serial: liteuart: add IRQ support

[Gabriel Somlo]

Add IRQ support to the LiteX LiteUART serial interface.

Gabriel Somlo (3):
  serial: liteuart: cosmetic changes
  serial: liteuart: separate RX loop from poll timer
  serial: liteuart: add IRQ support

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

-- 
2.37.3

[PATCH v1 1/3] serial: liteuart: cosmetic changes

[Gabriel Somlo]

Make some cosmetic/stylistic (non-functional) improvements:

1. Use a DRV_NAME macro to avoid hard-coding "liteuart" in multiple
locations throughout the source file

2. Use bit numbers instead of magic constants for event flags

3. Remove stub uart_ops methods that are not called unconditionally
from serial_core; Document stubs that are required by serial_core

4. Don't set unused port->regshift and port->iobase fields gratuitously
during probe()

5. Improve coding style in liteuart_init()

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

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 4c0604325ee9..4b9cca249828 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -18,6 +18,8 @@
 #include <linux/tty_flip.h>
 #include <linux/xarray.h>
 
+#define DRV_NAME "liteuart"
+
 /*
  * CSRs definitions (base address offsets + width)
  *
@@ -38,8 +40,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;
@@ -57,7 +59,7 @@ static struct console liteuart_console;
 
 static struct uart_driver liteuart_driver = {
 	.owner = THIS_MODULE,
-	.driver_name = "liteuart",
+	.driver_name = DRV_NAME,
 	.dev_name = "ttyLXU",
 	.major = 0,
 	.minor = 0,
@@ -122,6 +124,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 +157,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 +195,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 +221,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,
 };
@@ -280,9 +266,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);
@@ -322,7 +306,7 @@ static struct platform_driver liteuart_platform_driver = {
 	.probe = liteuart_probe,
 	.remove = liteuart_remove,
 	.driver = {
-		.name = "liteuart",
+		.name = DRV_NAME,
 		.of_match_table = liteuart_of_match,
 	},
 };
@@ -368,7 +352,7 @@ static int liteuart_console_setup(struct console *co, char *options)
 }
 
 static struct console liteuart_console = {
-	.name = "liteuart",
+	.name = DRV_NAME,
 	.write = liteuart_console_write,
 	.device = uart_console_device,
 	.setup = liteuart_console_setup,
@@ -416,12 +400,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

Re: [PATCH v1 1/3] serial: liteuart: cosmetic changes

[Greg KH]

On Mon, Nov 07, 2022 at 12:14:58PM -0500, Gabriel Somlo wrote:
> Make some cosmetic/stylistic (non-functional) improvements:
> 
> 1. Use a DRV_NAME macro to avoid hard-coding "liteuart" in multiple
> locations throughout the source file
> 
> 2. Use bit numbers instead of magic constants for event flags
> 
> 3. Remove stub uart_ops methods that are not called unconditionally
> from serial_core; Document stubs that are required by serial_core
> 
> 4. Don't set unused port->regshift and port->iobase fields gratuitously
> during probe()
> 
> 5. Improve coding style in liteuart_init()

When you list different things you do in a single patch, that means you
should break this up into individual patches.

Please do that here, this should be at least 5 patches.



> 
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  drivers/tty/serial/liteuart.c | 38 +++++++++--------------------------
>  1 file changed, 10 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 4c0604325ee9..4b9cca249828 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -18,6 +18,8 @@
>  #include <linux/tty_flip.h>
>  #include <linux/xarray.h>
>  
> +#define DRV_NAME "liteuart"

Just use KBUILD_MODNAME please.

thanks,

greg k-h

[PATCH v1 2/3] 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 4b9cca249828..90a29ed79bff 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -69,29 +69,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

Re: [PATCH v1 2/3] serial: liteuart: separate RX loop from poll timer

[Joel Stanley]

On Mon, 7 Nov 2022 at 17:15, Gabriel Somlo <gsomlo@gmail.com> 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 4b9cca249828..90a29ed79bff 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -69,29 +69,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;

u32, u8, void __iomem * would be better kernel types to use here.

You've also changed ch from a signed 32 to an unsigned 8.

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

You're no longer clearing EV_TX, but don't mention why (I understand
why with the context of the other changes, so perhaps add something to
this commit message).


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

Re: [PATCH v1 2/3] serial: liteuart: separate RX loop from poll timer

[Gabriel L. Somlo]

On Thu, Nov 10, 2022 at 01:09:59AM +0000, Joel Stanley wrote:
> On Mon, 7 Nov 2022 at 17:15, Gabriel Somlo <gsomlo@gmail.com> 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 4b9cca249828..90a29ed79bff 100644
> > --- a/drivers/tty/serial/liteuart.c
> > +++ b/drivers/tty/serial/liteuart.c
> > @@ -69,29 +69,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;
> 
> u32, u8, void __iomem * would be better kernel types to use here.
> 
> You've also changed ch from a signed 32 to an unsigned 8.

uart_insert_char() expects both `status` and `ch` to be of type
`unsigned int`. Switching `ch` to 8-bit was a typo, thanks for
catching it!

I'm going to use `unsigned int status, ch` in v3, to match the
signature of `uart_insert_char()` -- hope that's OK. This will be
a separate commit preceding the "move rx loop out of poll timer"
change.
 
> >
> >         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);
> 
> You're no longer clearing EV_TX, but don't mention why (I understand
> why with the context of the other changes, so perhaps add something to
> this commit message).

I'm adding a separate commit to document this (in v3) as well.

Thanks,
--Gabriel
 
> 
> >
> >                 /* 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 v1 3/3] 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 | 65 +++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 90a29ed79bff..47ce3ecc50f2 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>
@@ -90,13 +91,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));
 }
 
@@ -165,19 +180,48 @@ 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,
+				  DRV_NAME, port);
+		if (ret == 0) {
+			/* we only need interrupts on the rx path! */
+			irq_mask = EV_RX;
+		} else {
+			pr_err(DRV_NAME ": 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,
@@ -266,6 +310,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 v1 3/3] serial: liteuart: add IRQ support

[Joel Stanley]

On Mon, 7 Nov 2022 at 17:15, Gabriel Somlo <gsomlo@gmail.com> wrote:
>
> 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 | 65 +++++++++++++++++++++++++++++++----
>  1 file changed, 58 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 90a29ed79bff..47ce3ecc50f2 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>
> @@ -90,13 +91,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);

I don't follow this. If you've handled the RX IRQ, you want to return
IRQ_HANDLED. And if it's a different bit set you haven't handled it.

> +}
> +
>  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));
>  }
>
> @@ -165,19 +180,48 @@ 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,
> +                                 DRV_NAME, port);
> +               if (ret == 0) {
> +                       /* we only need interrupts on the rx path! */

Why not use the tx interrupts too?

> +                       irq_mask = EV_RX;
> +               } else {
> +                       pr_err(DRV_NAME ": can't attach LiteUART %d irq=%d; "
> +                              "switching to polling\n", port->line, port->irq);

put the string on the one line so it's grepable.

Take a look a the help for pr_fmt in include/linux/printk.h. This way
you get the driver name prefix for all pr_ messages.

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

Are you sure we need to take a lock and disable interrupts here?

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

same as above. I think the reason for doing this might have been if
you had a set of registers to change inside the critical section that
you needed to appear atomic. But this hardware only has one register
to flip, so we can do without the locking.

> +       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,
> @@ -266,6 +310,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 v1 3/3] serial: liteuart: add IRQ support

[Gabriel L. Somlo]

On Thu, Nov 10, 2022 at 01:08:55AM +0000, Joel Stanley wrote:
> On Mon, 7 Nov 2022 at 17:15, Gabriel Somlo <gsomlo@gmail.com> wrote:
> >
> > 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 | 65 +++++++++++++++++++++++++++++++----
> >  1 file changed, 58 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > index 90a29ed79bff..47ce3ecc50f2 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>
> > @@ -90,13 +91,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);
> 
> I don't follow this. If you've handled the RX IRQ, you want to return
> IRQ_HANDLED. And if it's a different bit set you haven't handled it.

Since I only enabled RX IRQs, it didn't occur to me that events would
still be generated for the TX path, regardless. I've made that clearer
(as an intermediate step) in the RX-path irq patch.
 
> > +}
> > +
> >  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));
> >  }
> >
> > @@ -165,19 +180,48 @@ 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,
> > +                                 DRV_NAME, port);
> > +               if (ret == 0) {
> > +                       /* we only need interrupts on the rx path! */
> 
> Why not use the tx interrupts too?

I've added another commit that also handles TX-mode IRQs (had to
rewrite the entire TX path to be compatible with both IRQ mode and
callable from the poll timer if IRQs are not supported).

> > +                       irq_mask = EV_RX;
> > +               } else {
> > +                       pr_err(DRV_NAME ": can't attach LiteUART %d irq=%d; "
> > +                              "switching to polling\n", port->line, port->irq);
> 
> put the string on the one line so it's grepable.
> 
> Take a look a the help for pr_fmt in include/linux/printk.h. This way
> you get the driver name prefix for all pr_ messages.

Got it, thanks!

> > +                       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);
> 
> Are you sure we need to take a lock and disable interrupts here?
> 
> > +       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);
> 
> same as above. I think the reason for doing this might have been if
> you had a set of registers to change inside the critical section that
> you needed to appear atomic. But this hardware only has one register
> to flip, so we can do without the locking.

Got rid of the locks in v3.

Thanks,
--Gabriel

> > +       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,
> > @@ -266,6 +310,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
> >