Re: [PATCHv2 3/5] xen/arm: Add the new OMAP UART driver.

[Julien Grall <julien.grall@linaro.org>]

On 08/07/2013 02:14 PM, Chen Baozi wrote:
> TI OMAP UART introduces some features such as register access modes, which
> makes its configuration and interrupt handling differs from 8250 compatible
> UART. Thus, we seperate this driver from ns16550's implementation.
> 
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>  config/arm32.mk               |   1 +
>  xen/common/device_tree.c      |  15 ++
>  xen/drivers/char/Makefile     |   1 +
>  xen/drivers/char/omap-uart.c  | 352 ++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/8250-uart.h   |  59 ++++++-
>  xen/include/xen/device_tree.h |   6 +
>  6 files changed, 431 insertions(+), 3 deletions(-)
>  create mode 100644 xen/drivers/char/omap-uart.c
> 
> diff --git a/config/arm32.mk b/config/arm32.mk
> index 8e21158..76e229d 100644
> --- a/config/arm32.mk
> +++ b/config/arm32.mk
> @@ -11,6 +11,7 @@ CFLAGS += -marm
>  
>  HAS_PL011 := y
>  HAS_EXYNOS4210 := y
> +HAS_OMAP := y
>  
>  # Use only if calling $(LD) directly.
>  LDFLAGS_DIRECT += -EL
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 84d704d..b392c60 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -574,6 +574,21 @@ const void *dt_get_property(const struct dt_device_node *np,
>      return pp ? pp->value : NULL;
>  }
>  
> +int dt_property_read_u32(const struct dt_device_node *np,
> +                            const char *name, u32 *out_value)
Alignment.

> +{
> +    u32 len;
> +    const __be32 *val;
> +
> +    val = dt_get_property(np, name, &len);
> +    if ( !val || len > 4 )
Please use sizeof(*out_val). It will avoid confusion later.

> +        return 1;
> +
> +    *out_value = be32_to_cpup(val);
> +
> +    return 0;
> +}
> +
>  bool_t dt_device_is_compatible(const struct dt_device_node *device,
>                                 const char *compat)
>  {
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index 37543f0..911b788 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -2,6 +2,7 @@ obj-y += console.o
>  obj-$(HAS_NS16550) += ns16550.o
>  obj-$(HAS_PL011) += pl011.o
>  obj-$(HAS_EXYNOS4210) += exynos4210-uart.o
> +obj-$(HAS_OMAP) += omap-uart.o
>  obj-$(HAS_EHCI) += ehci-dbgp.o
>  obj-$(CONFIG_ARM) += dt-uart.o
>  obj-y += serial.o
> diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
> new file mode 100644
> index 0000000..d5c2c34
> --- /dev/null
> +++ b/xen/drivers/char/omap-uart.c
> @@ -0,0 +1,352 @@
> +/*
> + * omap-uart.c
> + * Based on drivers/char/ns16550.c
> + *
> + * Driver for OMAP-UART contorller
controller
> + *
> + * Copyright (C) 2012, Chen Baozi <baozich@gmail.com>
> + *
> + * Note: This driver is made separate from 16550-series UART driver as
> + * omap platform has some specific configurations
> + */
> +
> +#include <xen/config.h>
> +#include <xen/console.h>
> +#include <xen/serial.h>
> +#include <xen/init.h>
> +#include <xen/irq.h>
> +#include <asm/early_printk.h>
> +#include <xen/device_tree.h>
> +#include <asm/device.h>
> +#include <xen/errno.h>
> +#include <xen/mm.h>
> +#include <xen/vmap.h>
> +#include <xen/8250-uart.h>
> +
> +#define omap_read(uart, off)       ioreadl((uart)->regs + (off<<REG_SHIFT))
> +#define omap_write(uart, off, val) iowritel((uart)->regs + (off<<REG_SHIFT), (val))
> +
> +static struct omap_uart {
> +    u32 baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
> +    struct dt_irq irq;
> +    void __iomem *regs;
> +    struct irqaction irqaction;
> +} omap_com = {0};
> +
> +static void omap_uart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
> +{
> +    struct serial_port *port = data;
> +    struct omap_uart *uart = port->uart;
> +    u32 lsr;
> +
> +    while (!(omap_read(uart, UART_IIR) & UART_IIR_NOINT))
Coding style. while ( ... )
> +    {
> +        lsr = omap_read(uart, UART_LSR) & 0xff;
> +	if (lsr & UART_LSR_THRE)

if ( ... )

Xen coding style doesn't allow hard tabs in the code, please replace it
by spaces.

> +	    serial_tx_interrupt(port, regs);
> +	if (lsr & UART_LSR_DR)
if ( ... ) and hard tab

> +	    serial_rx_interrupt(port, regs);

hard tab

> +
> +        if ( port->txbufc == port->txbufp )
> +            omap_write(uart, UART_IER, UART_IER_ERDAI|UART_IER_ELSI);
> +    };
> +}
> +
> +static void baud_protocol_setup(struct omap_uart *uart)
> +{
> +    u32 dll, dlh, efr;
> +    unsigned int divisor;
> +
> +    divisor = uart->clock_hz / (uart->baud << 4);
> +    dll = divisor & 0xff;
> +    dlh = divisor >> 8;
> +
> +    /*
> +     * Switch to register configuration mode B to access the UART_OMAP_EFR
> +     * register.
> +     */
> +    omap_write(uart, UART_LCR, UART_LCR_CONF_MODE_B);
> +    /*
> +     * Enable access to the UART_IER[7:4] bit field.
> +     */
> +    efr = omap_read(uart, UART_OMAP_EFR);
> +    omap_write(uart, UART_OMAP_EFR, efr|UART_OMAP_EFR_ECB);
> +    /*
> +     * Switch to register operation mode to access the UART_IER register.
> +     */
> +    omap_write(uart, UART_LCR, 0);
> +    /*
> +     * Clear the UART_IER register (set the UART_IER[4] SLEEP_MODE bit
> +     * to 0 to change the UART_DLL and UART_DLM register). Set the
> +     * UART_IER register value to 0x0000.
> +     */
> +    omap_write(uart, UART_IER, 0);
> +    /*
> +     * Switch to register configuartion mode B to access the UART_DLL and
> +     * UART_DLM registers.
> +     */
> +    omap_write(uart, UART_LCR, UART_LCR_CONF_MODE_B);
> +    /*
> +     * Load divisor value.
> +     */
> +    omap_write(uart, UART_DLL, dll);
> +    omap_write(uart, UART_DLM, dlh);
> +    /*
> +     * Restore the UART_OMAP_EFR
> +     */
> +    omap_write(uart, UART_OMAP_EFR, efr);
> +    /*
> +     * Load the new protocol formatting (parity, stop-bit, character length)
> +     * and switch to register operational mode.
> +     */
> +    omap_write(uart, UART_LCR, (uart->data_bits - 5) |
> +               ((uart->stop_bits - 1) << 2) | uart->parity);
> +}
> +
> +static void fifo_setup(struct omap_uart *uart)
> +{
> +    u32 lcr, efr, mcr;
> +    /*
> +     * Switch to register configuration mode B to access the UART_OMAP_EFR
> +     * register.
> +     */
> +    lcr = omap_read(uart, UART_LCR);
> +    omap_write(uart, UART_LCR, UART_LCR_CONF_MODE_B);
> +    /*
> +     * Enable register submode TCR_TLR to access the UART_OMAP_TLR register.
> +     */
> +    efr = omap_read(uart, UART_OMAP_EFR);
> +    omap_write(uart, UART_OMAP_EFR, efr|UART_OMAP_EFR_ECB);
> +    /*
> +     * Switch to register configuration mode A to access the UART_MCR
> +     * register.
> +     */
> +    omap_write(uart, UART_LCR, UART_LCR_CONF_MODE_A);
> +    /*
> +     * Enable register submode TCR_TLR to access the UART_OMAP_TLR register
> +     */
> +    mcr = omap_read(uart, UART_MCR);
> +    omap_write(uart, UART_MCR, mcr|UART_MCR_TCRTLR);
> +    /*
> +     * Enable the FIFO; load the new FIFO trigger and the new DMA mode.
> +     */
> +    omap_write(uart, UART_FCR, UART_FCR_R_TRIG_01|
> +               UART_FCR_T_TRIG_10|UART_FCR_ENABLE);
> +    /*
> +     * Switch to register configuration mode B to access the UART_EFR
> +     * register.
> +     */
> +    omap_write(uart, UART_LCR, UART_LCR_CONF_MODE_B);
> +    /*
> +     * Load the new FIFO triggers and the new DMA mode bit.
> +     */
> +    omap_write(uart, UART_OMAP_SCR, OMAP_UART_SCR_RX_TRIG_GRANU1_MASK);
> +    /*
> +     * Restore the UART_OMAP_EFR[4] value.
> +     */
> +    omap_write(uart, UART_OMAP_EFR, efr);
> +    /*
> +     * Switch to register configuration mode A to access the UART_MCR
> +     * register.
> +     */
> +    omap_write(uart, UART_LCR, UART_LCR_CONF_MODE_A);
> +    /*
> +     * Restore UART_MCR[6] value.
> +     */
> +    omap_write(uart, UART_MCR, mcr);
> +    /*
> +     * Restore UART_LCR value.
> +     */
> +    omap_write(uart, UART_LCR, lcr);
> +}
> +
> +static void __init omap_uart_init_preirq(struct serial_port *port)
> +{
> +    struct omap_uart *uart = port->uart;
> +
> +    /*
> +     * Clear the FIFO buffers.
> +     */
> +    omap_write(uart, UART_FCR, UART_FCR_ENABLE);
> +    omap_write(uart, UART_FCR, UART_FCR_ENABLE|UART_FCR_CLRX|UART_FCR_CLTX);
> +    omap_write(uart, UART_FCR, 0);
> +
> +    /*
> +     * The TRM says the mode should be disabled while UART_DLL and UART_DHL
> +     * are being changed so we disable before setup, then enable.
> +     */
> +    omap_write(uart, UART_OMAP_MDR1, UART_OMAP_MDR1_DISABLE);
> +
> +    /* Baud rate & protocol format setup */
> +    baud_protocol_setup(uart);
> +
> +    /* FIFO setup */
> +    fifo_setup(uart);
> +
> +    /* No flow control */
> +    omap_write(uart, UART_MCR, UART_MCR_DTR|UART_MCR_RTS);
> +
> +    omap_write(uart, UART_OMAP_MDR1, UART_OMAP_MDR1_16X_MODE);
> +
> +    uart->fifo_size = 64;

This assignment is related to the fifo. Could you move it in fifo_setup?

> +}
> +
> +static void __init omap_uart_init_postirq(struct serial_port *port)
> +{
> +    struct omap_uart *uart = port->uart;
> +
> +    uart->irqaction.handler = omap_uart_interrupt;
> +    uart->irqaction.name = "omap_uart";
> +    uart->irqaction.dev_id = port;
> +
> +    if ( setup_dt_irq(&uart->irq, &uart->irqaction) != 0 )
> +    {
> +        dprintk(XENLOG_ERR, "Failed to allocated omap_uart IRQ %d\n",
> +                uart->irq.irq);
> +        return;
> +    }
> +
> +    /* Enable interrupts */
> +    omap_write(uart, UART_IER, UART_IER_ERDAI|UART_IER_ETHREI|UART_IER_ELSI);
> +}
> +
> +static void omap_uart_suspend(struct serial_port *port)
> +{
> +    BUG();
> +}
> +
> +static void omap_uart_resume(struct serial_port *port)
> +{
> +    BUG();
> +}
> +
> +static unsigned int omap_uart_tx_ready(struct serial_port *port)
> +{
> +    struct omap_uart *uart = port->uart;
> +
> +    omap_write(uart, UART_IER, UART_IER_ERDAI|UART_IER_ETHREI|UART_IER_ELSI);
> +
> +    return omap_read(uart, UART_LSR) & UART_LSR_THRE ? uart->fifo_size : 0;
> +}
> +
> +static void omap_uart_putc(struct serial_port *port, char c)
> +{
> +    struct omap_uart *uart = port->uart;
> +
> +    omap_write(uart, UART_THR, (uint32_t)(unsigned char)c);
> +}
> +
> +static int omap_uart_getc(struct serial_port *port, char *pc)
> +{
> +    struct omap_uart *uart = port->uart;
> +
> +    if ( !(omap_read(uart, UART_LSR) & UART_LSR_DR) )
> +	return 0;
> +
> +    *pc = omap_read(uart, UART_RBR) & 0xff;
> +    return 1;
> +}
> +
> +static int __init omap_uart_irq(struct serial_port *port)
> +{
> +    struct omap_uart *uart = port->uart;
> +
> +    return ((uart->irq.irq > 0) ? uart->irq.irq : -1);
> +}
> +
> +static const struct dt_irq __init *omap_uart_dt_irq(struct serial_port *port)
> +{
> +    struct omap_uart *uart = port->uart;
> +
> +    return &uart->irq;
> +}
> +
> +static struct uart_driver __read_mostly omap_uart_driver = {
> +    .init_preirq = omap_uart_init_preirq,
> +    .init_postirq = omap_uart_init_postirq,
> +    .endboot = NULL,
> +    .suspend = omap_uart_suspend,
> +    .resume = omap_uart_resume,
> +    .tx_ready = omap_uart_tx_ready,
> +    .putc = omap_uart_putc,
> +    .getc = omap_uart_getc,
> +    .irq = omap_uart_irq,
> +    .dt_irq_get = omap_uart_dt_irq,
> +};
> +
> +static int __init omap_uart_init(struct dt_device_node *dev,
> +				 const void *data)
hard tab.

> +{
> +    const char *config = data;
> +    struct omap_uart *uart;
> +    u32 clkspec;
> +    int res;
> +    u64 addr, size;
> +
> +    if ( strcmp(config, "") )
> +        early_printk("WARNING: UART configuration is not supported\n");
> +
> +    uart = &omap_com;
> +
> +    res = dt_property_read_u32(dev, "clock-frequency", &clkspec);
> +    if ( res )
> +    {
> +        early_printk("omap-uart: Unable to retrieve the clock frequency\n");
> +        return -EINVAL;
> +    }
> +
> +    uart->clock_hz = clkspec;
> +    uart->baud = 115200;
> +    uart->data_bits = 8;
> +    uart->parity = UART_PARITY_NONE;
> +    uart->stop_bits = 1;
> +
> +    res = dt_device_get_address(dev, 0, &addr, &size);
> +    if ( res )
> +    {
> +        early_printk("omap-uart: Unable to retrieve the base"
> +                     " address of the UART\n");
> +        return res;
> +    }
> +
> +    uart->regs = ioremap_attr(addr, size, PAGE_HYPERVISOR_NOCACHE);
> +    if ( !uart->regs )
> +    {
> +        early_printk("omap-uart: Unable to map the UART memory\n");
> +        return -ENOMEM;
> +    }
> +
> +    res = dt_device_get_irq(dev, 0, &uart->irq);
> +    if ( res )
> +    {
> +        early_printk("omap-uart: Unable to retrieve the IRQ\n");
> +        return res;
> +    }
> +
> +    /* Register with generic serial driver */
> +    serial_register_uart(SERHND_DTUART, &omap_uart_driver, uart);
> +
> +    dt_device_set_used_by(dev, DOMID_XEN);
> +
> +    return 0;
> +}
> +
> +static const char const *omap_uart_dt_compat[] __initdata =
> +{
> +    "ti,omap4-uart",
> +    NULL
> +};
> +
> +DT_DEVICE_START(omap_uart, "OMAP UART", DEVICE_SERIAL)
> +    .compatible = omap_uart_dt_compat,
> +    .init = omap_uart_init,
> +DT_DEVICE_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> index 33daa6d..71970a1 100644
> --- a/xen/include/xen/8250-uart.h
> +++ b/xen/include/xen/8250-uart.h
> @@ -58,19 +58,51 @@
>  #define UART_FCR_CLRX     0x02    /* clear Rx FIFO        */
>  #define UART_FCR_CLTX     0x04    /* clear Tx FIFO        */
>  #define UART_FCR_DMA      0x10    /* enter DMA mode       */
> +
Spurious line.

>  #define UART_FCR_TRG1     0x00    /* Rx FIFO trig lev 1   */
>  #define UART_FCR_TRG4     0x40    /* Rx FIFO trig lev 4   */
>  #define UART_FCR_TRG8     0x80    /* Rx FIFO trig lev 8   */
>  #define UART_FCR_TRG14    0xc0    /* Rx FIFO trig lev 14  */
>  
> +/*
> + * Note: The FIFO trigger levels are chip specific:
> + *	RX:76 = 00  01  10  11	TX:54 = 00  01  10  11
> + * PC16550D:	 1   4   8  14		xx  xx  xx  xx
> + * TI16C550A:	 1   4   8  14          xx  xx  xx  xx
> + * TI16C550C:	 1   4   8  14          xx  xx  xx  xx
> + * ST16C550:	 1   4   8  14		xx  xx  xx  xx
> + * ST16C650:	 8  16  24  28		16   8  24  30	PORT_16650V2
> + * NS16C552:	 1   4   8  14		xx  xx  xx  xx
> + * ST16C654:	 8  16  56  60		 8  16  32  56	PORT_16654
> + * TI16C750:	 1  16  32  56		xx  xx  xx  xx	PORT_16750
> + * TI16C752:	 8  16  56  60		 8  16  32  56
> + * Tegra:	 1   4   8  14		16   8   4   1	PORT_TEGRA
> + */
> +#define UART_FCR_R_TRIG_00	0x00
> +#define UART_FCR_R_TRIG_01	0x40
> +#define UART_FCR_R_TRIG_10	0x80
> +#define UART_FCR_R_TRIG_11	0xc0
> +#define UART_FCR_T_TRIG_00	0x00
> +#define UART_FCR_T_TRIG_01	0x10
> +#define UART_FCR_T_TRIG_10	0x20
> +#define UART_FCR_T_TRIG_11	0x30
> +
>  /* Line Control Register */
>  #define UART_LCR_DLAB     0x80    /* Divisor Latch Access */
>  
> +/*
> + * Access to some registers depends on register access / configuration
> + * mode.
> + */
> +#define UART_LCR_CONF_MODE_A	UART_LCR_DLAB	/* Configuration mode A */
> +#define UART_LCR_CONF_MODE_B	0xBF		/* Configuration mode B */
> +
>  /* Modem Control Register */
> -#define UART_MCR_DTR      0x01    /* Data Terminal Ready  */
> -#define UART_MCR_RTS      0x02    /* Request to Send      */
> -#define UART_MCR_OUT2     0x08    /* OUT2: interrupt mask */
> +#define UART_MCR_TCRTLR   0x40    /* Access TCR/TLR (TI16C752, EFR[4]=1) */
>  #define UART_MCR_LOOP     0x10    /* Enable loopback test mode */
> +#define UART_MCR_OUT2     0x08    /* Out2 complement */
> +#define UART_MCR_RTS      0x02    /* RTS complement */
> +#define UART_MCR_DTR      0x01    /* DTR complement */
>  
>  /* Line Status Register */
>  #define UART_LSR_DR       0x01    /* Data ready           */
> @@ -96,6 +128,27 @@
>  #define RESUME_DELAY      MILLISECS(10)
>  #define RESUME_RETRIES    100
>  
> +/* Enhanced feature register */
> +#define UART_OMAP_EFR     0x02
> +
> +#define UART_OMAP_EFR_ECB 0x10 /* Enhanced control bit */
> +
> +/* Mode definition register 1 */
> +#define UART_OMAP_MDR1    0x08
> +
> +/*
> + * These are the definitions for the MDR1 register
> + */
> +#define UART_OMAP_MDR1_16X_MODE		0x00	/* UART 16x mode */
> +#define UART_OMAP_MDR1_DISABLE		0x07	/* Disable (default state) */
> +
> +/* Supplementary control register */
> +#define UART_OMAP_SCR     0x10
> +
> +/* SCR register bitmasks */
> +#define OMAP_UART_SCR_RX_TRIG_GRANU1_MASK (1 << 7)
> +
> +
>  #endif /* __XEN_8250_UART_H__ */
>  
>  /*
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 5a2a5c6..ab21a81 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -300,6 +300,12 @@ const void *dt_get_property(const struct dt_device_node *np,
>                              const char *name, u32 *lenp);
>  
>  /**
> + * dt_property_read_u32 - Helper to read a u32 property.
> + */
> +int dt_property_read_u32(const struct dt_device_node *np,
> +			 const char *name, u32 *out_value);

hard tab.

> +
> +/**
>   * Checks if the given "compat" string matches one of the strings in
>   * the device's "compatible" property
>   */
>