Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221107171500.2537938-1-gsomlo@gmail.com> <20221107171500.2537938-4-gsomlo@gmail.com>
In-Reply-To: <20221107171500.2537938-4-gsomlo@gmail.com>
From:   Joel Stanley <joel@jms.id.au>
Date:   Thu, 10 Nov 2022 01:08:55 +0000
Message-ID: <CACPK8XePY_nYo5OduxOAdAyUqtXju+9vAb+iXZqi6rmvR7Fp=Q@mail.gmail.com>
Subject: Re: [PATCH v1 3/3] serial: liteuart: add IRQ support
To:     Gabriel Somlo <gsomlo@gmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        gregkh@linuxfoundation.org, jirislaby@kernel.org,
        kgugala@antmicro.com, mholenko@antmicro.com,
        david.abdurachmanov@sifive.com, florent@enjoy-digital.fr
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>