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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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
> >