Add IRQ support to the LiteX LiteUART serial interface
Changes from v2:
- further split out "separate RX loop from poll timer" into
dedicated patches highlighting additional changes explicitly:
- factoring out tty_flip_buffer_push() (6/14)
- ack only RX events in RX loop (7/14)
- pass constant flag to uart_insert_char() directly (8/14)
- fix variable types in rx loop (9/14)
- separating RX loop from poll timer (10/14)
- added patch (11/14) to move function definitions to a more
convenient location, making subsequent changes easier to read
in diff patch form.
- fixes and clarifications for RX path IRQ support patch (now 12/14):
- only return IRQ_HANDLED for RX events
- use pr_fmt() to improve style of irq setup error message
- remove unnecessary locking from around single-register access
- modify TX path to support both IRQ-mode and polling (13/14)
- move polling-only liteuart_putchar() behind same conditional
(CONFIG_SERIAL_LITEUART_CONSOLE) as the rest of the code that's
still using it.
> Changes from v1:
> - split minor cosmetic changes out into individual patches
> (1/3 became 1..5/7)
> - patches 6/7 and 7/7 unchanged (used to be 2/3 and 3/3)
Gabriel Somlo (14):
serial: liteuart: use KBUILD_MODNAME as driver name
serial: liteuart: use bit number macros
serial: liteuart: remove unused uart_ops stubs
serial: liteuart: don't set unused port fields
serial: liteuart: minor style fix in liteuart_init()
serial: liteuart: move tty_flip_buffer_push() out of rx loop
serial: liteuart: rx loop should only ack rx events
serial: liteuart: simplify passing of uart_insert_char() flag
serial: liteuart: fix rx loop variable types
serial: liteuart: separate rx loop from poll timer
serial: liteuart: move function definitions
serial: liteuart: add IRQ support for the RX path
serial: liteuart: add IRQ support for the TX path
serial: liteuart: move polling putchar() function
drivers/tty/serial/liteuart.c | 226 +++++++++++++++++++++-------------
1 file changed, 142 insertions(+), 84 deletions(-)
--
2.37.3
Replace hard-coded instances of "liteuart" with KBUILD_MODNAME.
Signed-off-by: Gabriel Somlo <[email protected]>
---
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
Signed-off-by: Gabriel Somlo <[email protected]>
---
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
Add support for IRQ-driven RX. Support for the TX path will be added
in a separate commit.
Signed-off-by: Gabriel Somlo <[email protected]>
---
drivers/tty/serial/liteuart.c | 61 +++++++++++++++++++++++++++++++----
1 file changed, 54 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index cf1ce597b45e..e30adb30277f 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -6,6 +6,7 @@
*/
#include <linux/console.h>
+#include <linux/interrupt.h>
#include <linux/litex.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -130,13 +131,29 @@ static void liteuart_rx_chars(struct uart_port *port)
tty_flip_buffer_push(&port->state->port);
}
+static irqreturn_t liteuart_interrupt(int irq, void *data)
+{
+ struct liteuart_port *uart = data;
+ struct uart_port *port = &uart->port;
+ u8 isr = litex_read8(port->membase + OFF_EV_PENDING);
+
+ /* for now, only rx path triggers interrupts */
+ isr &= EV_RX;
+
+ spin_lock(&port->lock);
+ if (isr & EV_RX)
+ liteuart_rx_chars(port);
+ spin_unlock(&port->lock);
+
+ return IRQ_RETVAL(isr);
+}
+
static void liteuart_timer(struct timer_list *t)
{
struct liteuart_port *uart = from_timer(uart, t, timer);
struct uart_port *port = &uart->port;
- liteuart_rx_chars(port);
-
+ liteuart_interrupt(0, port);
mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
}
@@ -162,19 +179,42 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
static int liteuart_startup(struct uart_port *port)
{
struct liteuart_port *uart = to_liteuart_port(port);
+ int ret;
+ u8 irq_mask = 0;
- /* disable events */
- litex_write8(port->membase + OFF_EV_ENABLE, 0);
+ if (port->irq) {
+ ret = request_irq(port->irq, liteuart_interrupt, 0,
+ KBUILD_MODNAME, uart);
+ if (ret == 0) {
+ /* only enable rx interrupts at this time */
+ irq_mask = EV_RX;
+ } else {
+ pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
+ port->line, port->irq);
+ port->irq = 0;
+ }
+ }
- /* prepare timer for polling */
- timer_setup(&uart->timer, liteuart_timer, 0);
- mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
+ if (!port->irq) {
+ timer_setup(&uart->timer, liteuart_timer, 0);
+ mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
+ }
+
+ litex_write8(port->membase + OFF_EV_ENABLE, irq_mask);
return 0;
}
static void liteuart_shutdown(struct uart_port *port)
{
+ struct liteuart_port *uart = to_liteuart_port(port);
+
+ litex_write8(port->membase + OFF_EV_ENABLE, 0);
+
+ if (port->irq)
+ free_irq(port->irq, port);
+ else
+ del_timer_sync(&uart->timer);
}
static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
@@ -263,6 +303,13 @@ static int liteuart_probe(struct platform_device *pdev)
goto err_erase_id;
}
+ /* get irq */
+ ret = platform_get_irq_optional(pdev, 0);
+ if (ret < 0 && ret != -ENXIO)
+ return ret;
+ if (ret > 0)
+ port->irq = ret;
+
/* values not from device tree */
port->dev = &pdev->dev;
port->iotype = UPIO_MEM;
--
2.37.3
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 <[email protected]>
---
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
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 <[email protected]>
---
drivers/tty/serial/liteuart.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index b5ab48aa35cf..e9e99d6b5fef 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -72,7 +72,6 @@ static void liteuart_timer(struct timer_list *t)
struct liteuart_port *uart = from_timer(uart, t, timer);
struct uart_port *port = &uart->port;
unsigned char __iomem *membase = port->membase;
- unsigned int flg = TTY_NORMAL;
int ch;
unsigned long status;
@@ -85,7 +84,7 @@ static void liteuart_timer(struct timer_list *t)
/* no overflow bits in status */
if (!(uart_handle_sysrq_char(port, ch)))
- uart_insert_char(port, status, 0, ch, flg);
+ uart_insert_char(port, status, 0, ch, TTY_NORMAL);
}
tty_flip_buffer_push(&port->state->port);
--
2.37.3
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 <[email protected]>
---
drivers/tty/serial/liteuart.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 307c27398e30..767c356e60c9 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -69,14 +69,6 @@ static struct uart_driver liteuart_driver = {
#endif
};
-static void liteuart_putchar(struct uart_port *port, unsigned char ch)
-{
- while (litex_read8(port->membase + OFF_TXFULL))
- cpu_relax();
-
- litex_write8(port->membase + OFF_RXTX, ch);
-}
-
static void liteuart_stop_tx(struct uart_port *port)
{
if (port->irq) {
@@ -389,6 +381,14 @@ static struct platform_driver liteuart_platform_driver = {
#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
+static void liteuart_putchar(struct uart_port *port, unsigned char ch)
+{
+ while (litex_read8(port->membase + OFF_TXFULL))
+ cpu_relax();
+
+ litex_write8(port->membase + OFF_RXTX, ch);
+}
+
static void liteuart_console_write(struct console *co, const char *s,
unsigned int count)
{
--
2.37.3
On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> Signed-off-by: Gabriel Somlo <[email protected]>
> ---
> drivers/tty/serial/liteuart.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 5b684fd198b7..047d5ad32e13 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -398,12 +398,10 @@ static int __init liteuart_init(void)
> return res;
>
> res = platform_driver_register(&liteuart_platform_driver);
> - if (res) {
> + if (res)
> uart_unregister_driver(&liteuart_driver);
> - return res;
> - }
>
> - return 0;
> + return res;
> }
>
> static void __exit liteuart_exit(void)
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> Remove stub uart_ops methods that are not called unconditionally
> from serial_core. Document stubs that are expected to be present.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
> ---
> drivers/tty/serial/liteuart.c | 18 +-----------------
> 1 file changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 1497d4cdc221..90f6280c5452 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -122,6 +122,7 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
>
> static void liteuart_stop_tx(struct uart_port *port)
> {
> + /* not used in LiteUART, but called unconditionally from serial_core */
> }
>
> static void liteuart_start_tx(struct uart_port *port)
> @@ -154,11 +155,6 @@ static void liteuart_stop_rx(struct uart_port *port)
> del_timer(&uart->timer);
> }
>
> -static void liteuart_break_ctl(struct uart_port *port, int break_state)
> -{
> - /* LiteUART doesn't support sending break signal */
> -}
> -
> static int liteuart_startup(struct uart_port *port)
> {
> struct liteuart_port *uart = to_liteuart_port(port);
> @@ -197,15 +193,6 @@ static const char *liteuart_type(struct uart_port *port)
> return "liteuart";
> }
>
> -static void liteuart_release_port(struct uart_port *port)
> -{
> -}
> -
> -static int liteuart_request_port(struct uart_port *port)
> -{
> - return 0;
> -}
> -
> static void liteuart_config_port(struct uart_port *port, int flags)
> {
> /*
> @@ -232,13 +219,10 @@ static const struct uart_ops liteuart_ops = {
> .stop_tx = liteuart_stop_tx,
> .start_tx = liteuart_start_tx,
> .stop_rx = liteuart_stop_rx,
> - .break_ctl = liteuart_break_ctl,
> .startup = liteuart_startup,
> .shutdown = liteuart_shutdown,
> .set_termios = liteuart_set_termios,
> .type = liteuart_type,
> - .release_port = liteuart_release_port,
> - .request_port = liteuart_request_port,
> .config_port = liteuart_config_port,
> .verify_port = liteuart_verify_port,
> };
>
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> The polling liteuart_putchar() function is only called from methods
> conditionally enabled by CONFIG_SERIAL_LITEUART_CONSOLE. Move its
> definition closer to the console code where it is dependent on the
> same config option.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
> ---
> drivers/tty/serial/liteuart.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 307c27398e30..767c356e60c9 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -69,14 +69,6 @@ static struct uart_driver liteuart_driver = {
> #endif
> };
>
> -static void liteuart_putchar(struct uart_port *port, unsigned char ch)
> -{
> - while (litex_read8(port->membase + OFF_TXFULL))
> - cpu_relax();
> -
> - litex_write8(port->membase + OFF_RXTX, ch);
> -}
> -
> static void liteuart_stop_tx(struct uart_port *port)
> {
> if (port->irq) {
> @@ -389,6 +381,14 @@ static struct platform_driver liteuart_platform_driver = {
>
> #ifdef CONFIG_SERIAL_LITEUART_CONSOLE
>
> +static void liteuart_putchar(struct uart_port *port, unsigned char ch)
> +{
> + while (litex_read8(port->membase + OFF_TXFULL))
> + cpu_relax();
> +
> + litex_write8(port->membase + OFF_RXTX, ch);
> +}
> +
> static void liteuart_console_write(struct console *co, const char *s,
> unsigned int count)
> {
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> Simply provide the hard-coded TTY_NORMAL flag to uart_insert_char()
> directly -- no need to dedicate a variable for that exclusive purpose.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
> ---
> drivers/tty/serial/liteuart.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index b5ab48aa35cf..e9e99d6b5fef 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -72,7 +72,6 @@ static void liteuart_timer(struct timer_list *t)
> struct liteuart_port *uart = from_timer(uart, t, timer);
> struct uart_port *port = &uart->port;
> unsigned char __iomem *membase = port->membase;
> - unsigned int flg = TTY_NORMAL;
> int ch;
> unsigned long status;
>
> @@ -85,7 +84,7 @@ static void liteuart_timer(struct timer_list *t)
>
> /* no overflow bits in status */
> if (!(uart_handle_sysrq_char(port, ch)))
> - uart_insert_char(port, status, 0, ch, flg);
> + uart_insert_char(port, status, 0, ch, TTY_NORMAL);
> }
>
> tty_flip_buffer_push(&port->state->port);
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On Tue, 15 Nov 2022, Gabriel L. Somlo wrote:
> On Tue, Nov 15, 2022 at 06:00:11PM +0200, Ilpo J?rvinen wrote:
> > On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> >
> > > Add support for IRQ-driven RX. Support for the TX path will be added
> > > in a separate commit.
> > >
> > > Signed-off-by: Gabriel Somlo <[email protected]>
> > > ---
> > > drivers/tty/serial/liteuart.c | 61 +++++++++++++++++++++++++++++++----
> > > 1 file changed, 54 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > > index cf1ce597b45e..e30adb30277f 100644
> > > --- a/drivers/tty/serial/liteuart.c
> > > +++ b/drivers/tty/serial/liteuart.c
> > > @@ -6,6 +6,7 @@
> > > */
> > >
> > > #include <linux/console.h>
> > > +#include <linux/interrupt.h>
> > > #include <linux/litex.h>
> > > #include <linux/module.h>
> > > #include <linux/of.h>
> > > @@ -130,13 +131,29 @@ static void liteuart_rx_chars(struct uart_port *port)
> > > tty_flip_buffer_push(&port->state->port);
> > > }
> > >
> > > +static irqreturn_t liteuart_interrupt(int irq, void *data)
> > > +{
> > > + struct liteuart_port *uart = data;
> > > + struct uart_port *port = &uart->port;
> > > + u8 isr = litex_read8(port->membase + OFF_EV_PENDING);
> > > +
> > > + /* for now, only rx path triggers interrupts */
> >
> > Please don't add comment like this at all when your series removes it in a
> > later patch.
>
> OK, I will remove it in v4.
>
> > > + isr &= EV_RX;
> > > +
> > > + spin_lock(&port->lock);
> > > + if (isr & EV_RX)
> > > + liteuart_rx_chars(port);
> > > + spin_unlock(&port->lock);
> > > +
> > > + return IRQ_RETVAL(isr);
> > > +}
> > > +
> > > static void liteuart_timer(struct timer_list *t)
> > > {
> > > struct liteuart_port *uart = from_timer(uart, t, timer);
> > > struct uart_port *port = &uart->port;
> > >
> > > - liteuart_rx_chars(port);
> > > -
> > > + liteuart_interrupt(0, port);
> > > mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > > }
> > >
> > > @@ -162,19 +179,42 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
> > > static int liteuart_startup(struct uart_port *port)
> > > {
> > > struct liteuart_port *uart = to_liteuart_port(port);
> > > + int ret;
> > > + u8 irq_mask = 0;
> > >
> > > - /* disable events */
> > > - litex_write8(port->membase + OFF_EV_ENABLE, 0);
> > > + if (port->irq) {
> > > + ret = request_irq(port->irq, liteuart_interrupt, 0,
> > > + KBUILD_MODNAME, uart);
> > > + if (ret == 0) {
> > > + /* only enable rx interrupts at this time */
> >
> > This comment seems pretty useless. Your code says very much the same.
>
> The comment was meant to let the reader know that the code is doing it
> *intentionally* (rather than forgetting to enable tx irqs by mistake).
> But I'm OK with removing this comment in v4 as well if you think
> that's an overly paranoid and redundant thing to do... :)
I see. Reading the other comment first caused me to misinterpret this one
to mean that only RX interrupts are implemented.
Maybe if you change "at this time" to "at startup" it would make it more
obvious.
--
i.
On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> Add support for IRQ-driven RX. Support for the TX path will be added
> in a separate commit.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
> ---
> drivers/tty/serial/liteuart.c | 61 +++++++++++++++++++++++++++++++----
> 1 file changed, 54 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index cf1ce597b45e..e30adb30277f 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -6,6 +6,7 @@
> */
>
> #include <linux/console.h>
> +#include <linux/interrupt.h>
> #include <linux/litex.h>
> #include <linux/module.h>
> #include <linux/of.h>
> @@ -130,13 +131,29 @@ static void liteuart_rx_chars(struct uart_port *port)
> tty_flip_buffer_push(&port->state->port);
> }
>
> +static irqreturn_t liteuart_interrupt(int irq, void *data)
> +{
> + struct liteuart_port *uart = data;
> + struct uart_port *port = &uart->port;
> + u8 isr = litex_read8(port->membase + OFF_EV_PENDING);
> +
> + /* for now, only rx path triggers interrupts */
Please don't add comment like this at all when your series removes it in a
later patch.
> + isr &= EV_RX;
> +
> + spin_lock(&port->lock);
> + if (isr & EV_RX)
> + liteuart_rx_chars(port);
> + spin_unlock(&port->lock);
> +
> + return IRQ_RETVAL(isr);
> +}
> +
> static void liteuart_timer(struct timer_list *t)
> {
> struct liteuart_port *uart = from_timer(uart, t, timer);
> struct uart_port *port = &uart->port;
>
> - liteuart_rx_chars(port);
> -
> + liteuart_interrupt(0, port);
> mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> }
>
> @@ -162,19 +179,42 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
> static int liteuart_startup(struct uart_port *port)
> {
> struct liteuart_port *uart = to_liteuart_port(port);
> + int ret;
> + u8 irq_mask = 0;
>
> - /* disable events */
> - litex_write8(port->membase + OFF_EV_ENABLE, 0);
> + if (port->irq) {
> + ret = request_irq(port->irq, liteuart_interrupt, 0,
> + KBUILD_MODNAME, uart);
> + if (ret == 0) {
> + /* only enable rx interrupts at this time */
This comment seems pretty useless. Your code says very much the same.
--
i.
> + irq_mask = EV_RX;
> + } else {
> + pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
> + port->line, port->irq);
> + port->irq = 0;
> + }
> + }
>
> - /* prepare timer for polling */
> - timer_setup(&uart->timer, liteuart_timer, 0);
> - mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> + if (!port->irq) {
> + timer_setup(&uart->timer, liteuart_timer, 0);
> + mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> + }
> +
> + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask);
>
> return 0;
> }
>
> static void liteuart_shutdown(struct uart_port *port)
> {
> + struct liteuart_port *uart = to_liteuart_port(port);
> +
> + litex_write8(port->membase + OFF_EV_ENABLE, 0);
> +
> + if (port->irq)
> + free_irq(port->irq, port);
> + else
> + del_timer_sync(&uart->timer);
> }
>
> static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
> @@ -263,6 +303,13 @@ static int liteuart_probe(struct platform_device *pdev)
> goto err_erase_id;
> }
>
> + /* get irq */
> + ret = platform_get_irq_optional(pdev, 0);
> + if (ret < 0 && ret != -ENXIO)
> + return ret;
> + if (ret > 0)
> + port->irq = ret;
> +
> /* values not from device tree */
> port->dev = &pdev->dev;
> port->iotype = UPIO_MEM;
>
On Tue, Nov 15, 2022 at 06:21:00PM +0200, Ilpo J?rvinen wrote:
> On Tue, 15 Nov 2022, Gabriel L. Somlo wrote:
>
> > On Tue, Nov 15, 2022 at 06:00:11PM +0200, Ilpo J?rvinen wrote:
> > > On Sat, 12 Nov 2022, Gabriel Somlo wrote:
> > >
> > > > Add support for IRQ-driven RX. Support for the TX path will be added
> > > > in a separate commit.
> > > >
> > > > Signed-off-by: Gabriel Somlo <[email protected]>
> > > > ---
> > > > drivers/tty/serial/liteuart.c | 61 +++++++++++++++++++++++++++++++----
> > > > 1 file changed, 54 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > > > index cf1ce597b45e..e30adb30277f 100644
> > > > --- a/drivers/tty/serial/liteuart.c
> > > > +++ b/drivers/tty/serial/liteuart.c
> > > > @@ -6,6 +6,7 @@
> > > > */
> > > >
> > > > #include <linux/console.h>
> > > > +#include <linux/interrupt.h>
> > > > #include <linux/litex.h>
> > > > #include <linux/module.h>
> > > > #include <linux/of.h>
> > > > @@ -130,13 +131,29 @@ static void liteuart_rx_chars(struct uart_port *port)
> > > > tty_flip_buffer_push(&port->state->port);
> > > > }
> > > >
> > > > +static irqreturn_t liteuart_interrupt(int irq, void *data)
> > > > +{
> > > > + struct liteuart_port *uart = data;
> > > > + struct uart_port *port = &uart->port;
> > > > + u8 isr = litex_read8(port->membase + OFF_EV_PENDING);
> > > > +
> > > > + /* for now, only rx path triggers interrupts */
> > >
> > > Please don't add comment like this at all when your series removes it in a
> > > later patch.
> >
> > OK, I will remove it in v4.
> >
> > > > + isr &= EV_RX;
> > > > +
> > > > + spin_lock(&port->lock);
> > > > + if (isr & EV_RX)
> > > > + liteuart_rx_chars(port);
> > > > + spin_unlock(&port->lock);
> > > > +
> > > > + return IRQ_RETVAL(isr);
> > > > +}
> > > > +
> > > > static void liteuart_timer(struct timer_list *t)
> > > > {
> > > > struct liteuart_port *uart = from_timer(uart, t, timer);
> > > > struct uart_port *port = &uart->port;
> > > >
> > > > - liteuart_rx_chars(port);
> > > > -
> > > > + liteuart_interrupt(0, port);
> > > > mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > > > }
> > > >
> > > > @@ -162,19 +179,42 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
> > > > static int liteuart_startup(struct uart_port *port)
> > > > {
> > > > struct liteuart_port *uart = to_liteuart_port(port);
> > > > + int ret;
> > > > + u8 irq_mask = 0;
> > > >
> > > > - /* disable events */
> > > > - litex_write8(port->membase + OFF_EV_ENABLE, 0);
> > > > + if (port->irq) {
> > > > + ret = request_irq(port->irq, liteuart_interrupt, 0,
> > > > + KBUILD_MODNAME, uart);
> > > > + if (ret == 0) {
> > > > + /* only enable rx interrupts at this time */
> > >
> > > This comment seems pretty useless. Your code says very much the same.
> >
> > The comment was meant to let the reader know that the code is doing it
> > *intentionally* (rather than forgetting to enable tx irqs by mistake).
> > But I'm OK with removing this comment in v4 as well if you think
> > that's an overly paranoid and redundant thing to do... :)
>
> I see. Reading the other comment first caused me to misinterpret this one
> to mean that only RX interrupts are implemented.
>
> Maybe if you change "at this time" to "at startup" it would make it more
> obvious.
OK, I'll fix the comment per your suggestion rather than get rid of it.
Thanks again,
--G
> --
> i.
On Tue, Nov 15, 2022 at 06:00:11PM +0200, Ilpo J?rvinen wrote:
> On Sat, 12 Nov 2022, Gabriel Somlo wrote:
>
> > Add support for IRQ-driven RX. Support for the TX path will be added
> > in a separate commit.
> >
> > Signed-off-by: Gabriel Somlo <[email protected]>
> > ---
> > drivers/tty/serial/liteuart.c | 61 +++++++++++++++++++++++++++++++----
> > 1 file changed, 54 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > index cf1ce597b45e..e30adb30277f 100644
> > --- a/drivers/tty/serial/liteuart.c
> > +++ b/drivers/tty/serial/liteuart.c
> > @@ -6,6 +6,7 @@
> > */
> >
> > #include <linux/console.h>
> > +#include <linux/interrupt.h>
> > #include <linux/litex.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > @@ -130,13 +131,29 @@ static void liteuart_rx_chars(struct uart_port *port)
> > tty_flip_buffer_push(&port->state->port);
> > }
> >
> > +static irqreturn_t liteuart_interrupt(int irq, void *data)
> > +{
> > + struct liteuart_port *uart = data;
> > + struct uart_port *port = &uart->port;
> > + u8 isr = litex_read8(port->membase + OFF_EV_PENDING);
> > +
> > + /* for now, only rx path triggers interrupts */
>
> Please don't add comment like this at all when your series removes it in a
> later patch.
OK, I will remove it in v4.
> > + isr &= EV_RX;
> > +
> > + spin_lock(&port->lock);
> > + if (isr & EV_RX)
> > + liteuart_rx_chars(port);
> > + spin_unlock(&port->lock);
> > +
> > + return IRQ_RETVAL(isr);
> > +}
> > +
> > static void liteuart_timer(struct timer_list *t)
> > {
> > struct liteuart_port *uart = from_timer(uart, t, timer);
> > struct uart_port *port = &uart->port;
> >
> > - liteuart_rx_chars(port);
> > -
> > + liteuart_interrupt(0, port);
> > mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > }
> >
> > @@ -162,19 +179,42 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
> > static int liteuart_startup(struct uart_port *port)
> > {
> > struct liteuart_port *uart = to_liteuart_port(port);
> > + int ret;
> > + u8 irq_mask = 0;
> >
> > - /* disable events */
> > - litex_write8(port->membase + OFF_EV_ENABLE, 0);
> > + if (port->irq) {
> > + ret = request_irq(port->irq, liteuart_interrupt, 0,
> > + KBUILD_MODNAME, uart);
> > + if (ret == 0) {
> > + /* only enable rx interrupts at this time */
>
> This comment seems pretty useless. Your code says very much the same.
The comment was meant to let the reader know that the code is doing it
*intentionally* (rather than forgetting to enable tx irqs by mistake).
But I'm OK with removing this comment in v4 as well if you think
that's an overly paranoid and redundant thing to do... :)
Thanks,
--Gabriel
> --
> i.
>
> > + irq_mask = EV_RX;
> > + } else {
> > + pr_err(pr_fmt("line %d irq %d failed: using polling\n"),
> > + port->line, port->irq);
> > + port->irq = 0;
> > + }
> > + }
> >
> > - /* prepare timer for polling */
> > - timer_setup(&uart->timer, liteuart_timer, 0);
> > - mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > + if (!port->irq) {
> > + timer_setup(&uart->timer, liteuart_timer, 0);
> > + mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > + }
> > +
> > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask);
> >
> > return 0;
> > }
> >
> > static void liteuart_shutdown(struct uart_port *port)
> > {
> > + struct liteuart_port *uart = to_liteuart_port(port);
> > +
> > + litex_write8(port->membase + OFF_EV_ENABLE, 0);
> > +
> > + if (port->irq)
> > + free_irq(port->irq, port);
> > + else
> > + del_timer_sync(&uart->timer);
> > }
> >
> > static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
> > @@ -263,6 +303,13 @@ static int liteuart_probe(struct platform_device *pdev)
> > goto err_erase_id;
> > }
> >
> > + /* get irq */
> > + ret = platform_get_irq_optional(pdev, 0);
> > + if (ret < 0 && ret != -ENXIO)
> > + return ret;
> > + if (ret > 0)
> > + port->irq = ret;
> > +
> > /* values not from device tree */
> > port->dev = &pdev->dev;
> > port->iotype = UPIO_MEM;
> >
>