Re: [PATCH v2 4/4] drivers/tty/serial: add LiteUART driver

[Mateusz Holenko <mholenko@antmicro.com>]

sob., 26 paź 2019 o 02:13 Rob Herring <robh@kernel.org> napisał(a):
>
> On Wed, Oct 23, 2019 at 11:47:23AM +0200, Mateusz Holenko wrote:
> > From: Filip Kokosinski <fkokosinski@internships.antmicro.com>
> >
> > This commit adds driver for the FPGA-based LiteUART serial controller
> > from LiteX SoC builder.
> >
> > The current implementation supports LiteUART configured
> > for 32 bit data width and 8 bit CSR bus width.
> >
> > It does not support IRQ.
> >
> > Signed-off-by: Filip Kokosinski <fkokosinski@internships.antmicro.com>
> > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > ---
> > Changes in v2:
> > - used register access functions from newly introduced litex.h
> > - patch number changed from 3 to 4
> >
> >  MAINTAINERS                      |   1 +
> >  drivers/tty/serial/Kconfig       |  30 +++
> >  drivers/tty/serial/Makefile      |   1 +
> >  drivers/tty/serial/liteuart.c    | 373 +++++++++++++++++++++++++++++++
> >  include/uapi/linux/serial_core.h |   3 +
> >  5 files changed, 408 insertions(+)
> >  create mode 100644 drivers/tty/serial/liteuart.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1dc783c9edb7..c24a37833e78 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9499,6 +9499,7 @@ M:      Mateusz Holenko <mholenko@antmicro.com>
> >  S:   Maintained
> >  F:   include/linux/litex.h
> >  F:   Documentation/devicetree/bindings/*/litex,*.yaml
> > +F:   drivers/tty/serial/liteuart.c
> >
> >  LIVE PATCHING
> >  M:   Josh Poimboeuf <jpoimboe@redhat.com>
> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > index 4789b5d62f63..b01fe12a1411 100644
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -1571,6 +1571,36 @@ config SERIAL_MILBEAUT_USIO_CONSOLE
> >         receives all kernel messages and warnings and which allows logins in
> >         single user mode).
> >
> > +config SERIAL_LITEUART
> > +     tristate "LiteUART serial port support"
> > +     depends on HAS_IOMEM
> > +     depends on OF
> > +     select SERIAL_CORE
> > +     help
> > +       This driver is for the FPGA-based LiteUART serial controller from LiteX
> > +       SoC builder.
> > +
> > +       Say 'Y' here if you wish to use the LiteUART serial controller.
> > +       Otherwise, say 'N'.
> > +
> > +config SERIAL_LITEUART_NR_PORTS
> > +     int "Number of LiteUART ports"
> > +     depends on SERIAL_LITEUART
> > +     default "1"
> > +     help
> > +       Set this to the number of serial ports you want the driver
> > +       to support.
>
> It's better if the driver supports a dynamic number of ports.

Do you mean that the number of ports should be taken from the device
tree when booting the kernel?
This is exactly how it works right now. The SERIAL_LITEUART_NR_PORTS just
defines the maximum number of supported ports.
I can rename the config to SERIAL_LITEUART_MAX_NR_PORTS to make it
clearer, what do you think?

> > +
> > +config SERIAL_LITEUART_CONSOLE
> > +     bool "LiteUART serial port console support"
> > +     depends on SERIAL_LITEUART=y
> > +     select SERIAL_CORE_CONSOLE
> > +     help
> > +       Say 'Y' here if you wish to use the FPGA-based LiteUART serial controller
> > +       from LiteX SoC builder as the system console (the system console is the
> > +       device which receives all kernel messages and warnings and which allows
> > +       logins in single user mode). Otherwise, say 'N'.
> > +
> >  endmenu
> >
> >  config SERIAL_MCTRL_GPIO
> > diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> > index 863f47056539..c8d7e2046284 100644
> > --- a/drivers/tty/serial/Makefile
> > +++ b/drivers/tty/serial/Makefile
> > @@ -89,6 +89,7 @@ obj-$(CONFIG_SERIAL_OWL)    += owl-uart.o
> >  obj-$(CONFIG_SERIAL_RDA)     += rda-uart.o
> >  obj-$(CONFIG_SERIAL_MILBEAUT_USIO) += milbeaut_usio.o
> >  obj-$(CONFIG_SERIAL_SIFIVE)  += sifive.o
> > +obj-$(CONFIG_SERIAL_LITEUART) += liteuart.o
> >
> >  # GPIOLIB helpers for modem control lines
> >  obj-$(CONFIG_SERIAL_MCTRL_GPIO)      += serial_mctrl_gpio.o
> > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > new file mode 100644
> > index 000000000000..e142f78df57a
> > --- /dev/null
> > +++ b/drivers/tty/serial/liteuart.c
> > @@ -0,0 +1,373 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LiteUART serial controller (LiteX) Driver
> > + *
> > + * Copyright (C) 2019 Antmicro Ltd <www.antmicro.com>
> > + */
> > +
> > +#include <linux/console.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/serial.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/timer.h>
> > +#include <linux/tty_flip.h>
> > +#include <linux/litex.h>
> > +
> > +/* module-related defines */
> > +#define DRIVER_NAME  "liteuart"
> > +#define DRIVER_MAJOR 0
> > +#define DRIVER_MINOR 0
> > +#define DEV_NAME     "ttyLXU"
> > +
> > +/* base address offsets */
> > +#define OFF_RXTX     0x00
> > +#define OFF_TXFULL   0x04
> > +#define OFF_RXEMPTY  0x08
> > +#define OFF_EV_STATUS        0x0c
> > +#define OFF_EV_PENDING       0x10
> > +#define OFF_EV_ENABLE        0x14
> > +
> > +/* events */
> > +#define EV_TX        0x1
> > +#define EV_RX        0x2
> > +
> > +struct liteuart_port {
> > +     struct uart_port port;
> > +     struct timer_list timer;
> > +};
> > +
> > +#define to_liteuart_port(port)       container_of(port, struct liteuart_port, port)
> > +
> > +static struct liteuart_port liteuart_ports[CONFIG_SERIAL_LITEUART_NR_PORTS];
> > +
> > +#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
> > +static struct console liteuart_console;
> > +#endif
> > +
> > +static struct uart_driver liteuart_driver = {
> > +     .owner = THIS_MODULE,
> > +     .driver_name = DRIVER_NAME,
> > +     .dev_name = DEV_NAME,
> > +     .major = DRIVER_MAJOR,
> > +     .minor = DRIVER_MINOR,
> > +     .nr = CONFIG_SERIAL_LITEUART_NR_PORTS,
> > +#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
> > +     .cons = &liteuart_console,
> > +#endif
> > +};
> > +
> > +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 int status;
> > +
> > +     while ((status = !LITEX_READ_REG_OFF(membase, OFF_RXEMPTY)) == 1) {
> > +             ch = LITEX_READ_REG_OFF(membase, OFF_RXTX);
> > +             port->icount.rx++;
> > +
> > +             /* necessary for RXEMPTY to refresh its value */
> > +             LITEX_WRITE_REG_OFF(EV_TX | EV_RX, membase, OFF_EV_PENDING);
> > +
> > +             /* no overflow bits in status */
> > +             if (!(uart_handle_sysrq_char(port, ch)))
> > +                     uart_insert_char(port, status, 0, ch, flg);
> > +
> > +             tty_flip_buffer_push(&port->state->port);
> > +     }
> > +
> > +     mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > +}
> > +
> > +static void liteuart_putchar(struct uart_port *port, int ch)
> > +{
> > +     while (LITEX_READ_REG_OFF(port->membase, OFF_TXFULL))
> > +             cpu_relax();
> > +
> > +     LITEX_WRITE_REG_OFF(ch, port->membase, OFF_RXTX);
> > +}
> > +
> > +static unsigned int liteuart_tx_empty(struct uart_port *port)
> > +{
> > +     /* not really tx empty, just checking if tx is not full */
> > +     if (!LITEX_READ_REG_OFF(port->membase, OFF_TXFULL))
> > +             return TIOCSER_TEMT;
> > +
> > +     return 0;
> > +}
> > +
> > +static void liteuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
> > +{
> > +     /* modem control register is not present in LiteUART */
> > +}
> > +
> > +static unsigned int liteuart_get_mctrl(struct uart_port *port)
> > +{
> > +     return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
> > +}
> > +
> > +static void liteuart_stop_tx(struct uart_port *port)
> > +{
> > +}
> > +
> > +static void liteuart_start_tx(struct uart_port *port)
> > +{
> > +     struct circ_buf *xmit = &port->state->xmit;
> > +     unsigned char ch;
> > +
> > +     if (unlikely(port->x_char)) {
> > +             LITEX_WRITE_REG_OFF(port->x_char, port->membase, OFF_RXTX);
> > +             port->icount.tx++;
> > +             port->x_char = 0;
> > +     } else if (!uart_circ_empty(xmit)) {
> > +             while (xmit->head != xmit->tail) {
> > +                     ch = xmit->buf[xmit->tail];
> > +                     xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> > +                     port->icount.tx++;
> > +                     liteuart_putchar(port, ch);
> > +             }
> > +     }
> > +
> > +     if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> > +             uart_write_wakeup(port);
> > +}
> > +
> > +static void liteuart_stop_rx(struct uart_port *port)
> > +{
> > +     struct liteuart_port *uart = to_liteuart_port(port);
> > +
> > +     /* just delete timer */
> > +     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);
> > +
> > +     /* disable events */
> > +     LITEX_WRITE_REG_OFF(0, port->membase, OFF_EV_ENABLE);
> > +
> > +     /* prepare timer for polling */
> > +     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)
> > +{
> > +}
> > +
> > +static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
> > +                              struct ktermios *old)
> > +{
> > +     unsigned int baud;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&port->lock, flags);
> > +
> > +     /* update baudrate */
> > +     baud = uart_get_baud_rate(port, new, old, 0, 460800);
> > +     uart_update_timeout(port, new->c_cflag, baud);
> > +
> > +     spin_unlock_irqrestore(&port->lock, flags);
> > +}
> > +
> > +static const char *liteuart_type(struct uart_port *port)
> > +{
> > +     return (port->type == PORT_LITEUART) ? DRIVER_NAME : NULL;
> > +}
> > +
> > +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)
> > +{
> > +     if (flags & UART_CONFIG_TYPE)
> > +             port->type = PORT_LITEUART;
> > +}
> > +
> > +static int liteuart_verify_port(struct uart_port *port,
> > +                             struct serial_struct *ser)
> > +{
> > +     if (port->type != PORT_UNKNOWN && ser->type != PORT_LITEUART)
> > +             return -EINVAL;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct uart_ops liteuart_ops = {
> > +     .tx_empty       = liteuart_tx_empty,
> > +     .set_mctrl      = liteuart_set_mctrl,
> > +     .get_mctrl      = liteuart_get_mctrl,
> > +     .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,
> > +};
> > +
> > +static int liteuart_probe(struct platform_device *pdev)
> > +{
> > +     struct device_node *np = pdev->dev.of_node;
> > +     struct liteuart_port *uart;
> > +     struct uart_port *port;
> > +     int dev_id;
> > +
> > +     /* no device tree */
> > +     if (!np)
> > +             return -ENODEV;
> > +
> > +     dev_id = of_alias_get_id(np, "serial");
> > +     if (dev_id < 0 || dev_id >= CONFIG_SERIAL_LITEUART_NR_PORTS)
> > +             return -EINVAL;
>
> aliases should be optional. There's a helper to get a free index without
> an alias you can use.

Sure, we will fix it in v4.


-- 
Mateusz Holenko
Antmicro Ltd | www.antmicro.com
Roosevelta 22, 60-829 Poznan, Poland