|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/5] xen/arm: Add the new OMAP UART driver.
On Aug 7, 2013, at 3:14 AM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> On 5 August 2013 12:49, Chen Baozi <baozich@xxxxxxxxx> 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.
>
> On your previous version of this patch series you use a modified ns16550,
> why didn't you continue in the same way?
I used to think that OMAP UART is 8250 compatible with just a few
additional registers. However, since I read the manual carefully last
weekend, I found it rather different than a common 8250 UART,
especially switching different register access modes to do
do initialization and disable THRE interrupt after finishing tx.
Those features would make the modified ns16550 share less common
codes between X86 and ARM. What's more, OMAP's UART codes cannot be
reused on other ARM platform such as suni6. So I think it would
be better to separate it into a new driver and leave ns16550 as the
driver for a more common 8250 UART such as suni6.
>
>> Signed-off-by: Chen Baozi <baozich@xxxxxxxxx>
>> ---
>> config/arm32.mk | 1 +
>> xen/drivers/char/Makefile | 1 +
>> xen/drivers/char/omap-uart.c | 358
>> +++++++++++++++++++++++++++++++++++++++++++
>> xen/include/xen/8250-uart.h | 74 ++++++++-
>> 4 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/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..a8cbcc7
>> --- /dev/null
>> +++ b/xen/drivers/char/omap-uart.c
>> @@ -0,0 +1,358 @@
>> +/*
>> + * omap-uart.c
>> + * Based on drivers/char/ns16550.c
>> + *
>> + * Driver for OMAP-UART contorller
>> + *
>> + * Copyright (C) 2012, Chen Baozi <baozich@xxxxxxxxx>
>> + *
>> + * 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>
>> +
>> +static struct omap_uart {
>> + u32 baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
>> + struct dt_irq irq;
>> + volatile uint32_t *regs;
>> + struct irqaction irqaction;
>> +
>> + u32 dll;
>> + u32 ier;
>> + u32 dlh;
>> + u32 fcr;
>> + u32 efr;
>> + u32 lcr;
>> + u32 mcr;
>> + u32 mdr1;
>> + u32 scr;
>> +
>> +} 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;
>> +
>> + do
>> + {
>
> Why do you change the while by a do-while?
Hmmm, it is due to the intermediate implementation of this function.
I used to check LSR status in the loop statement, where lsr should
be loaded before reaching the statement.
After times of debugging/modifications, it comes to be the situation
as present, but keep the old do-while structure...
If there are any coding-style problems, I'm OK to change it to the while…
>
>> + lsr = uart->regs[UART_LSR] & 0xff;
>
> Please use ioread{l,w,...} instead of uart->regs[...]. Same for
> everywhere in you driver.
No problem.
Any potential hazard using uart->regs[...]?
>
>> + if (lsr & UART_LSR_THRE)
>> + serial_tx_interrupt(port, regs);
>> + if (lsr & UART_LSR_DR)
>> + serial_rx_interrupt(port, regs);
>> +
>> + if ( port->txbufc == port->txbufp ) {
> Unnecessary bracket.
>> + uart->regs[UART_IER] = UART_IER_ERDAI | UART_IER_ELSI;
>> + }
>> + } while (!(uart->regs[UART_IIR] & UART_IIR_NOINT));
>> +
>> +
>> +}
>> +
>> +static void baud_protocol_setup(struct omap_uart *uart)
>> +{
>> + /*
>> + * Switch to register configuration mode B to access the UART_OMAP_EFR
>> + * register.
>> + */
>> + uart->regs[UART_LCR] = UART_LCR_CONF_MODE_B;
>> + /*
>> + * Enable access to the UART_IER[7:4] bit field.
>> + */
>> + uart->efr = uart->regs[UART_OMAP_EFR];
>> + uart->regs[UART_OMAP_EFR] = uart->efr | UART_OMAP_EFR_ECB;
>> + /*
>> + * Switch to register operation mode to access the UART_IER register.
>> + */
>> + uart->regs[UART_LCR] = 0x0;
>> + /*
>> + * 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.
>> + */
>> + uart->regs[UART_IER] = 0x0;
>> + /*
>> + * Switch to register configuartion mode B to access the UART_DLL and
>> + * UART_DLM registers.
>> + */
>> + uart->regs[UART_LCR] = UART_LCR_CONF_MODE_B;
>> + /*
>> + * Load divisor value.
>> + */
>> + uart->regs[UART_DLL] = uart->dll;
>> + uart->regs[UART_DLM] = uart->dlh;
>> + /*
>> + * Restore the UART_OMAP_EFR
>> + */
>> + uart->regs[UART_OMAP_EFR] = uart->efr;
>> + /*
>> + * Load the new protocol formatting (parity, stop-bit, character length)
>> + * and switch to register operational mode.
>> + */
>> + uart->regs[UART_LCR] = uart->lcr;
>> +}
>> +
>> +static void fifo_setup(struct omap_uart *uart)
>> +{
>> + /*
>> + * Switch to register configuration mode B to access the UART_OMAP_EFR
>> + * register.
>> + */
>> + uart->lcr = uart->regs[UART_LCR];
>> + uart->regs[UART_LCR] = UART_LCR_CONF_MODE_B;
>> + /*
>> + * Enable register submode TCR_TLR to access the UART_OMAP_TLR register.
>> + */
>> + uart->efr = uart->regs[UART_OMAP_EFR];
>> + uart->regs[UART_OMAP_EFR] = uart->efr | UART_OMAP_EFR_ECB;
>> + /*
>> + * Switch to register configuration mode A to access the UART_MCR
>> + * register.
>> + */
>> + uart->regs[UART_LCR] = UART_LCR_CONF_MODE_A;
>> + /*
>> + * Enable register submode TCR_TLR to access the UART_OMAP_TLR register
>> + */
>> + uart->mcr = uart->regs[UART_MCR];
>> + uart->regs[UART_MCR] = uart->mcr | UART_MCR_TCRTLR;
>> + /*
>> + * Enable the FIFO; load the new FIFO trigger and the new DMA mode.
>> + */
>> + uart->regs[UART_FCR] = uart->fcr;
>> + /*
>> + * Switch to register configuration mode B to access the UART_EFR
>> + * register.
>> + */
>> + uart->regs[UART_LCR] = UART_LCR_CONF_MODE_B;
>> + /*
>> + * Load the new FIFO triggers and the new DMA mode bit.
>> + */
>> + uart->regs[UART_OMAP_SCR] = uart->scr;
>> + /*
>> + * Restore the UART_OMAP_EFR[4] value.
>> + */
>> + uart->regs[UART_OMAP_EFR] = uart->efr;
>> + /*
>> + * Switch to register configuration mode A to access the UART_MCR
>> + * register.
>> + */
>> + uart->regs[UART_LCR] = UART_LCR_CONF_MODE_A;
>> + /*
>> + * Restore UART_MCR[6] value.
>> + */
>> + uart->regs[UART_MCR] = uart->mcr;
>> + /*
>> + * Restore UART_LCR value.
>> + */
>> + uart->regs[UART_LCR] = uart->lcr;
>> +}
>> +
>> +static void __init omap_uart_init_preirq(struct serial_port *port)
>> +{
>> + struct omap_uart *uart = port->uart;
>> + unsigned int divisor;
>> +
>> + /*
>> + * Clear the FIFO buffers.
>> + */
>> + uart->regs[UART_FCR] = UART_FCR_ENABLE;
>> + uart->regs[UART_FCR] = UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX;
>> + uart->regs[UART_FCR] = 0;
>> +
>> + /*
>> + * Calculate desired value.
>> + */
>> + divisor = uart->clock_hz / (uart->baud << 4);
>> + uart->dll = divisor & 0xff;
>> + uart->dlh = divisor >> 8;
>> + uart->lcr = (uart->data_bits - 5) | ((uart->stop_bits - 1) << 2) |
>> uart->parity;
>> + uart->fcr = UART_FCR_R_TRIG_01 | UART_FCR_T_TRIG_10 | UART_FCR_ENABLE;
>> + uart->scr = OMAP_UART_SCR_RX_TRIG_GRANU1_MASK;
>> + uart->ier = UART_IER_ERDAI | UART_IER_ETHREI | UART_IER_ELSI;
>
> You are wasting space just to store hardcoded value (for fcr, scr,
> ier). You should use macro.
>
>> +
>> + /*
>> + * The TRM says the mode should be disabled while UART_DLL and UART_DHL
>> + * are being changed so we disable before setup, then enable.
>> + */
>> + uart->regs[UART_OMAP_MDR1] = UART_OMAP_MDR1_DISABLE;
>> +
>> + /* Baud rate & protocol format setup */
>> + baud_protocol_setup(uart);
>> +
>> + /* FIFO setup */
>> + fifo_setup(uart);
>> +
>> + /* No flow control */
>> + uart->regs[UART_MCR] = UART_MCR_DTR | UART_MCR_RTS;
>> +
>> + uart->regs[UART_OMAP_MDR1] = UART_OMAP_MDR1_16X_MODE;
>> +
>> + uart->fifo_size = 64;
>> +}
>> +
>> +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)
>
> Missing space for the if.
> if ( a ) ...
>
>> + dprintk(XENLOG_ERR, "Failed to allocated omap_uart IRQ %d\n",
>> + uart->irq.irq);
>> +
>> + /* Enable interrupts */
>> + uart->regs[UART_IER] = uart->ier;
>
> Don't enable IRQ if setup has failed.
>
>> +}
>> +
>> +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;
>> +
>> + uart->regs[UART_IER] = uart->ier;
>> +
>> + return uart->regs[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;
>> +
>> + uart->regs[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 (!(uart->regs[UART_LSR] & UART_LSR_DR))
> Missing space for the if.
>
>> + return 0;
>> +
>> + *pc = uart->regs[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)
>> +{
>> + const char *config = data;
>> + struct omap_uart *uart;
>> + const __be32 *clkspec;
>> + int res;
>> + u64 addr, size;
>> +
>> + if (strcmp(config, ""))
>> + early_printk("WARNING: UART configuration is not supported\n");
>> +
>> + uart = &omap_com;
>> +
>> + clkspec = dt_get_property(dev, "clock-frequency", NULL);
>> + if (clkspec == NULL) {
>> + early_printk("omap-uart: Unable to retrieve the clock frequency\n");
>> + return -EINVAL;
>> + }
>> +
>> + uart->clock_hz = be32_to_cpup(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) {
> The bracket needs to be on a new line.
>> + 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) {
>
> Missing space and bracket in newline.
>
>> + early_printk("omap-uart: Unable to map the UART memory\n");
>> + return -ENOMEM;
>> + }
>> +
>> + res = dt_device_get_irq(dev, 0, &uart->irq);
>> + if (res) {
> Same here.
>> + 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 0e6c6bd..a75f36a 100644
>> --- a/xen/include/xen/8250-uart.h
>> +++ b/xen/include/xen/8250-uart.h
>> @@ -60,19 +60,55 @@
>> #define UART_FCR_CLRX 0x02 /* clear Rx FIFO */
>> #define UART_FCR_CLTX 0x04 /* clear Tx FIFO */
>> #define UART_FCR_DMA 0x10 /* enter DMA mode */
>> +
>> #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 /* Configutation mode A */
>> +#define UART_LCR_CONF_MODE_B 0xBF /* Configutation 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_CLKSEL 0x80 /* Divide clock by 4 (TI16C752, EFR[4]=1)
>> */
>> +#define UART_MCR_TCRTLR 0x40 /* Access TCR/TLR (TI16C752, EFR[4]=1) */
>> +#define UART_MCR_XONANY 0x20 /* Enable Xon Any (TI16C752, EFR[4]=1) */
>
> Don't add unnecessary define.
>
>> +#define UART_MCR_AFE 0x20 /* Enable auto-RTS/CTS
>> (TI16C550C/TI16C750) */
>> #define UART_MCR_LOOP 0x10 /* Enable loopback test mode */
>> +#define UART_MCR_OUT2 0x08 /* Out2 complement */
>> +#define UART_MCR_OUT1 0x04 /* Out1 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 */
>> @@ -98,6 +134,38 @@
>> #define RESUME_DELAY MILLISECS(10)
>> #define RESUME_RETRIES 100
>>
>> +/* Enhanced feature register */
>> +#define UART_OMAP_EFR 0x02
>> +
>> +#define UART_OMAP_EFR_CTS 0x80 /* CTS flow control */
>> +#define UART_OMAP_EFR_RTS 0x40 /* RTS flow control */
>> +#define UART_OMAP_EFR_SCD 0x20 /* Special character detect */
>> +#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_SIR_MODE 0x01 /* SIR mode */
>> +#define UART_OMAP_MDR1_16X_ABAUD_MODE 0x02 /* UART 16x auto-baud */
>> +#define UART_OMAP_MDR1_13X_MODE 0x03 /* UART 13x mode */
>> +#define UART_OMAP_MDR1_MIR_MODE 0x04 /* MIR mode */
>> +#define UART_OMAP_MDR1_FIR_MODE 0x05 /* FIR mode */
>> +#define UART_OMAP_MDR1_CIR_MODE 0x06 /* CIR 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)
>> +#define OMAP_UART_SCR_TX_TRIG_GRANU1_MASK (1 << 6)
>> +#define OMAP_UART_SCR_TX_EMPTY (1 << 3)
>> +
>> +
>> #endif /* __XEN_8250_UART_H__ */
>>
>> /*
Thanks for reviewing. I'll rework and resend it later.
Cheers,
Baozi
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |