diff mbox series

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

Message ID 20221118145512.509950-13-gsomlo@gmail.com
State New
Headers
Series serial: liteuart: add IRQ support |

Commit Message

Gabriel L. Somlo Nov. 18, 2022, 2:55 p.m. UTC 
  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(-)

Comments

Jiri Slaby Nov. 21, 2022, 8:54 a.m. UTC | #1 
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,
Gabriel L. Somlo Nov. 21, 2022, 6:50 p.m. UTC | #2 
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
>
Jiri Slaby Nov. 22, 2022, 7:44 a.m. UTC | #3 
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,
Geert Uytterhoeven Nov. 22, 2022, 9:54 a.m. UTC | #4 
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
Gabriel L. Somlo Nov. 22, 2022, 7:41 p.m. UTC | #5 
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
diff mbox series

Patch

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;