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

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

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

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