Re: [PATCH v5] OMAP UART: Add omap-serial driver support.

Govindraj.R
2010-02-04T15:09:14Z

Hi Olof,

>On Thu, Jan 28, 2010 at 8:52 AM, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
> Thanks for reposting this, it's been a while since the last review round
> so I've taken a closer look at it.
>
> Lots of comments below.
>
>
> -Olof
>
> On Wed, Jan 27, 2010 at 03:26:56PM +0530, Govindraj.R wrote:
>> >From 869dde60756b2115c35d97f8e2daf014cb1af08e Mon Sep 17 00:00:00 2001
>> From: Govindraj R <govindraj.raja@ti.com>
>> Date: Wed, 27 Jan 2010 14:57:58 +0530
>> Subject: [PATCH] OMAP UART: Add omap-serial driver support.
>>
>> This patch adds support for OMAP-HIGH SPEED UART Controller.
>>
>> It adds support for the following features:
>> 1. It supports Interrupt mode and DMA mode of operation.
>> 2. Supports Hardware flow control and software flow control.
>> 3. Debug Console support on all UARTs.
>>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>> Signed-off-by: Govindraj R <govindraj.raja@ti.com>
>> ---
>>  arch/arm/plat-omap/include/plat/omap-serial.h |  128 +++
>>  drivers/serial/Kconfig                        |   23 +
>>  drivers/serial/Makefile                       |    1 +
>>  drivers/serial/omap-serial.c                  | 1349 +++++++++++++++++++++++++
>>  include/linux/serial_core.h                   |    3 +
>>  5 files changed, 1504 insertions(+), 0 deletions(-)
>>  create mode 100755 arch/arm/plat-omap/include/plat/omap-serial.h
>>  create mode 100755 drivers/serial/omap-serial.c
>>
>> diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h
>
>> @@ -0,0 +1,128 @@
>> +/*
>> + * Driver for OMAP-UART controller.
>> + * Based on drivers/serial/8250.c
>> + *
>> + * Copyright (C) 2009 Texas Instruments.
>
> It's considered polite to duplicate the original copyright after the "based on" text.
>
> Also, 8250.c is v2-or-later, and this is v2-only. You shouldn't change the license
> without the original author's approval (i.e. it's more hassle than it is worth).

Will retain as specified in 8250 driver.

>
>> + *
>> + * Authors:
>> + *   Govindraj R     <govindraj.raja@ti.com>
>> + *   Thara Gopinath  <thara@ti.com>
>> + *
>> + * This file is licensed under the terms of the GNU General Public License
>> + * version 2. This program is licensed "as is" without any warranty of any
>> + * kind, whether express or implied.
>> + */
>> +
>> +#ifndef __OMAP_SERIAL_H__
>> +#define __OMAP_SERIAL_H__
>> +
>> +#include <linux/serial_core.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include <plat/control.h>
>> +#include <plat/mux.h>
>> +
>> +#define DRIVER_NAME  "omap-hsuart"
>> +
>> +/*
>> + * Use tty device name as ttyO, [O -> OMAP]
>> + * in bootargs we specify as console=ttyO0 if uart1
>> + * is used as console uart.
>> + * Use Major 204 and minor 64.
>> + * This is necessary if we should coexist with the 8250 driver,
>> + * if we have an external TI-16C750 UART. Ex.ZOOM2/3 Boards.
>> + */
>
> Comments should document _why_ you do something, not _what_ you're
> doing. As such, the "Use Major 204 and minor 64" is redundant, it's
> obvious from looking at the lines below.
>
>> +
>> +#define OMAP_SERIAL_NAME     "ttyO"
>> +#define OMAP_SERIAL_MAJOR    204
>> +#define OMAP_SERIAL_MINOR    64
>
> Where did these numbers come from? 204/64 are listed as in use in
> Documentation/devices.txt. 213 is the first available minor there. (You
> should also update that file).
>

I was referring to some custom uart soc driver files.
Most of them seemed to use in an similar fashion,

References:
drivers/serial/samsung.c
drivers/serial/timbuart.c
drivers/serial/uartlite.c

And many more which co-exist with 8250 driver.
Use in an similar way.

>> +
>> +/*
>> + * We default to IRQ0 for the "no irq" hack.   Some
>> + * machine types want others as well - they're free
>> + * to redefine this in their header file.
>> + */
>> +
>> +#define is_real_interrupt(irq)  ((irq) != 0)
>
> You should compare to NO_IRQ instead.
>
>> +
>> +#if defined(CONFIG_SERIAL_OMAP_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
>> +#define SUPPORT_SYSRQ
>> +#endif
>> +
>> +#define OMAP_MDR1_DISABLE    0x07
>> +#define OMAP_MDR1_MODE13X    0x03
>> +#define OMAP_MDR1_MODE16X    0x00
>> +#define OMAP_MODE13X_SPEED   230400
>> +
>> +#define CONSOLE_NAME "console="
>> +
>> +#define RX_TIMEOUT   (3 * HZ)
>> +
>> +#ifdef CONFIG_ARCH_OMAP4
>> +#define OMAP_MAX_HSUART_PORTS 4
>> +#else
>> +#define OMAP_MAX_HSUART_PORTS 3
>> +#endif
>> +
>> +struct omap_uart_port_info {
>> +     bool                    dma_enabled;    /* To specify DMA Mode */
>> +     unsigned int            uartclk;        /* UART clock rate */
>> +     void __iomem            *membase;       /* ioremap cookie or NULL */
>> +     resource_size_t         mapbase;        /* resource base */
>> +     unsigned long           irqflags;       /* request_irq flags */
>> +     upf_t                   flags;          /* UPF_* flags */
>> +     };
>> +
>> +struct uart_omap_dma {
>> +     u8                      uart_dma_tx;
>> +     u8                      uart_dma_rx;
>> +     int                     rx_dma_channel;
>> +     int                     tx_dma_channel;
>> +     dma_addr_t              rx_buf_dma_phys;
>> +     dma_addr_t              tx_buf_dma_phys;
>> +     /*
>> +      * Buffer for rx dma.It is not required for tx because the buffer
>> +      * comes from port structure
>> +      */
>> +     unsigned int            uart_base;
>> +     unsigned char           *rx_buf;
>> +     unsigned int            prev_rx_dma_pos;
>> +     int                     tx_buf_size;
>> +     int                     tx_dma_state;
>> +     int                     rx_dma_state;
>> +     spinlock_t              tx_lock;
>> +     spinlock_t              rx_lock;
>> +     /* timer to poll activity on rx dma */
>> +     struct timer_list       rx_timer;
>> +     int                     rx_buf_size;
>> +     int                     rx_timeout;
>> +};
>> +
>> +struct uart_omap_port {
>> +     struct uart_port        port;
>> +     struct uart_omap_dma    uart_dma;
>> +     struct platform_device  *pdev;
>> +
>> +     unsigned char           ier;
>> +     unsigned char           lcr;
>> +     unsigned char           mcr;
>> +     unsigned char           fcr;
>> +     unsigned char           efr;
>> +
>> +     int                     use_dma;
>> +     int                     is_buf_dma_alloced;
>
>
> Alloced is bad english. :)  "allocated" would be better.

Will correct it.

>
>> +     /*
>> +      * Some bits in registers are cleared on a read, so they must
>> +      * be saved whenever the register is read but the bits will not
>> +      * be immediately processed.
>> +      */
>> +     unsigned int            lsr_break_flag;
>> +#define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
>
> I know the 8250 code does the define in the structure, but it still seems
> like a random thing to do.  Please move it up with the other defines.

Will move this from the structure.

>
>> +     unsigned char           msr_saved_flags;
>> +     char                    name[20];
>> +     spinlock_t              uart_lock;
>> +     unsigned long           port_activity;
>> +};
>> +
>> +extern char *saved_command_line;
>> +#endif /* __OMAP_SERIAL_H__ */
>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
>> index 9ff47db..a5b1c71 100644
>> --- a/drivers/serial/Kconfig
>> +++ b/drivers/serial/Kconfig
>> @@ -1366,6 +1366,29 @@ config SERIAL_OF_PLATFORM
>>         Currently, only 8250 compatible ports are supported, but
>>         others can easily be added.
>>
>> +config SERIAL_OMAP
>> +     tristate "OMAP serial port support"
>> +     depends on ARCH_OMAP3 || ARCH_OMAP4
>> +     select SERIAL_CORE
>> +     help
>> +     If you have a machine based on an Texas Instruments OMAP CPU you
>> +     can enable its onboard serial ports by enabling this option.
>> +
>> +config SERIAL_OMAP_CONSOLE
>> +     bool "Console on OMAP serial port"
>> +     depends on SERIAL_OMAP
>> +     select SERIAL_CORE_CONSOLE
>> +     help
>> +     If you have enabled the serial port on the Texas Instruments OMAP
>> +     CPU you can make it the console by answering Y to this option.
>> +
>> +     Even if you say Y here, the currently visible virtual console
>> +     (/dev/tty0) will still be used as the system console by default, but
>> +     you can alter that using a kernel command line option such as
>> +     "console=tty0". (Try "man bootparam" or see the documentation of
>> +     your boot loader (lilo or loadlin) about how to pass options to the
>> +     kernel at boot time.)
>
> ARM has neither lilo nor loadlin. The help text could do with a minor cleanup.
>

will correct this.

>> +
>>  config SERIAL_OF_PLATFORM_NWPSERIAL
>>       tristate "NWP serial port driver"
>>       depends on PPC_OF && PPC_DCR
>> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
>> index 5548fe7..42962b3 100644
>> --- a/drivers/serial/Makefile
>> +++ b/drivers/serial/Makefile
>> @@ -82,3 +82,4 @@ obj-$(CONFIG_KGDB_SERIAL_CONSOLE) += kgdboc.o
>>  obj-$(CONFIG_SERIAL_QE) += ucc_uart.o
>>  obj-$(CONFIG_SERIAL_TIMBERDALE)      += timbuart.o
>>  obj-$(CONFIG_SERIAL_GRLIB_GAISLER_APBUART) += apbuart.o
>> +obj-$(CONFIG_SERIAL_OMAP) += omap-serial.o
>> diff --git a/drivers/serial/omap-serial.c b/drivers/serial/omap-serial.c
>> new file mode 100755
>> index 0000000..72697ca
>> --- /dev/null
>> +++ b/drivers/serial/omap-serial.c
>> @@ -0,0 +1,1349 @@
>> +/*
>> + * Driver for OMAP-UART controller.
>> + * Based on drivers/serial/8250.c
>> + *
>> + * Copyright (C) 2009 Texas Instruments.
>> + *
>> + * Authors:
>> + *   Govindraj R     <govindraj.raja@ti.com>
>> + *   Thara Gopinath  <thara@ti.com>
>> + *
>> + * This file is licensed under the terms of the GNU General Public License
>> + * version 2. This program is licensed "as is" without any warranty of any
>> + * kind, whether express or implied.
>> + *
>> + * Note: This driver is made seperate from 8250 driver as we cannot
>> + * over load 8250 driver with omap platform specific configuration for
>> + * features like DMA, it makes easier to implement features like DMA and
>> + * hardware flow control and software flow control configuration with
>> + * this driver as required for the omap-platform.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/console.h>
>> +#include <linux/serial_reg.h>
>> +#include <linux/delay.h>
>> +#include <linux/tty.h>
>> +#include <linux/tty_flip.h>
>> +#include <linux/io.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/clk.h>
>> +#include <linux/serial_core.h>
>> +
>> +#include <asm/irq.h>
>> +#include <plat/dma.h>
>> +#include <plat/dmtimer.h>
>> +#include <plat/omap-serial.h>
>> +
>> +static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
>> +
>> +/* Forward declaration of functions */
>> +static void uart_tx_dma_callback(int lch, u16 ch_status, void *data);
>> +static void serial_omap_rx_timeout(unsigned long uart_no);
>> +static void serial_omap_start_rxdma(struct uart_omap_port *up);
>> +static int serial_omap_remove(struct platform_device *dev);
>> +
>> +int console_detect(char *str)
>> +{
>> +     char *next, *start = NULL;
>> +     int i;
>> +
>> +     i = strlen(CONSOLE_NAME);
>> +     next = saved_command_line;
>> +
>> +     while ((next = strchr(next, 'c')) != NULL) {
>> +             if (!strncmp(next, CONSOLE_NAME, i)) {
>> +                     start = next;
>> +                     break;
>> +             } else {
>> +                     next++;
>> +             }
>> +     }
>> +     if (!start)
>> +             return -EPERM;
>> +     i = 0;
>> +     start = strchr(start, '=') + 1;
>> +     while (*start != ',') {
>> +             str[i++] = *start++;
>> +             if (i > 6) {
>> +                     pr_err("%s : Invalid Console Name\n", __func__);
>> +                     return -EPERM;
>> +             }
>> +     }
>> +     str[i] = '\0';
>> +     return 0;
>> +}
>
> What's the purpose of this function? console_setup() will already select
> the right console for you, right?

Yes correct, will remove this one.

>
>> +
>> +static inline unsigned int serial_in(struct uart_omap_port *up, int offset)
>> +{
>> +     offset <<= up->port.regshift;
>> +     return readw(up->port.membase + offset);
>> +}
>> +
>> +static inline void serial_out(struct uart_omap_port *up, int offset, int value)
>> +{
>> +     offset <<= up->port.regshift;
>> +     writew(value, up->port.membase + offset);
>> +}
>> +
>> +static inline void serial_omap_clear_fifos(struct uart_omap_port *port)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +
>> +     serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
>> +     serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO |
>> +                    UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
>> +     serial_out(up, UART_FCR, 0);
>> +}
>> +
>> +/**
>> + * serial_omap_get_divisor() - calculate divisor value
>> + * @port: uart port info
>> + * @baud: baudrate for which divisor needs to be calculated.
>> + *
>> + * We have written our own function to get the divisor so as to support
>> + * 13x mode.
>> + */
>
> Again, the why, not the how. Why do you need the 13x divisor? What's
> magic about 3Mbaud?
>

Refering to TRM UART chapter 17:

Table 17-1. UART Mode Baud Rates, Divisor Values, and Error Rates

referring to oversampling - divisor value

baudrate 460,800 to 3,686,400 all have divisor 13

except 3,000,000 which has divisor value 16

thus we are checking if baud != 3000000

>> +static unsigned int
>> +serial_omap_get_divisor(struct uart_port *port, unsigned int baud)
>> +{
>> +     unsigned int divisor;
>> +
>> +     if (baud > OMAP_MODE13X_SPEED && baud != 3000000)
>> +             divisor = 13;
>> +     else
>> +             divisor = 16;
>> +     return port->uartclk/(baud * divisor);
>> +}
>> +
>> +static void serial_omap_stop_rxdma(struct uart_omap_port *up)
>> +{
>> +     if (up->uart_dma.rx_dma_state) {
>> +             del_timer_sync(&up->uart_dma.rx_timer);
>> +             omap_stop_dma(up->uart_dma.rx_dma_channel);
>> +             omap_free_dma(up->uart_dma.rx_dma_channel);
>> +             up->uart_dma.rx_dma_channel = 0xFF;
>> +             up->uart_dma.rx_dma_state = 0x0;
>> +     }
>> +}
>> +
>> +static void serial_omap_enable_ms(struct uart_port *port)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +
>> +     dev_dbg(up->port.dev, "serial_omap_enable_ms+%d\n", up->pdev->id);
>> +     up->ier |= UART_IER_MSI;
>> +     serial_out(up, UART_IER, up->ier);
>> +}
>> +
>> +static void serial_omap_stop_tx(struct uart_port *port)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +
>> +     if (up->use_dma) {
>> +             if (up->uart_dma.tx_dma_channel != 0xFF) {
>
> Could be nice to have a define for this magic value.

Will add an macro.

>
>> +                     /*
>> +                      * Check if dma is still active . If yes do nothing,
>> +                      * return. Else stop dma
>> +                      */
>> +                     if (omap_get_dma_active_status
>> +                                     (up->uart_dma.tx_dma_channel))
>> +                             return;
>> +                     omap_stop_dma(up->uart_dma.tx_dma_channel);
>> +                     omap_free_dma(up->uart_dma.tx_dma_channel);
>> +                     up->uart_dma.tx_dma_channel = 0xFF;
>> +             }
>> +     }
>> +
>> +     if (up->ier & UART_IER_THRI) {
>> +             up->ier &= ~UART_IER_THRI;
>> +             serial_out(up, UART_IER, up->ier);
>> +     }
>> +}
>> +
>> +static void serial_omap_stop_rx(struct uart_port *port)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +
>> +     serial_omap_stop_rxdma(up);
>> +     up->ier &= ~UART_IER_RLSI;
>> +     up->port.read_status_mask &= ~UART_LSR_DR;
>> +     serial_out(up, UART_IER, up->ier);
>> +}
>> +
>> +static inline void receive_chars(struct uart_omap_port *up, int *status)
>> +{
>> +     struct tty_struct *tty = up->port.state->port.tty;
>> +     unsigned int flag;
>> +     unsigned char ch, lsr = *status;
>> +     int max_count = 256;
>> +
>> +     do {
>> +             if (likely(lsr & UART_LSR_DR))
>> +                     ch = serial_in(up, UART_RX);
>> +             flag = TTY_NORMAL;
>> +             up->port.icount.rx++;
>> +
>> +             if (unlikely(lsr & UART_LSR_BRK_ERROR_BITS)) {
>> +                     /*
>> +                      * For statistics only
>> +                      */
>> +                     if (lsr & UART_LSR_BI) {
>> +                             lsr &= ~(UART_LSR_FE | UART_LSR_PE);
>> +                             up->port.icount.brk++;
>> +                             /*
>> +                              * We do the SysRQ and SAK checking
>> +                              * here because otherwise the break
>> +                              * may get masked by ignore_status_mask
>> +                              * or read_status_mask.
>> +                              */
>> +                             if (uart_handle_break(&up->port))
>> +                                     goto ignore_char;
>> +                     } else if (lsr & UART_LSR_PE)
>> +                             up->port.icount.parity++;
>> +                     else if (lsr & UART_LSR_FE)
>> +                             up->port.icount.frame++;
>> +                     if (lsr & UART_LSR_OE)
>> +                             up->port.icount.overrun++;
>> +
>> +                     /*
>> +                      * Mask off conditions which should be ignored.
>> +                      */
>> +                     lsr &= up->port.read_status_mask;
>> +
>> +#ifdef CONFIG_SERIAL_OMAP_CONSOLE
>> +                     if (up->port.line == up->port.cons->index) {
>> +                             /* Recover the break flag from console xmit */
>> +                             lsr |= up->lsr_break_flag;
>> +                             up->lsr_break_flag = 0;
>> +                     }
>> +#endif
>> +                     if (lsr & UART_LSR_BI)
>> +                             flag = TTY_BREAK;
>> +                     else if (lsr & UART_LSR_PE)
>> +                             flag = TTY_PARITY;
>> +                     else if (lsr & UART_LSR_FE)
>> +                             flag = TTY_FRAME;
>> +             }
>> +
>> +             if (uart_handle_sysrq_char(&up->port, ch))
>> +                     goto ignore_char;
>> +             uart_insert_char(&up->port, lsr, UART_LSR_OE, ch, flag);
>> +ignore_char:
>> +             lsr = serial_in(up, UART_LSR);
>> +     } while ((lsr & (UART_LSR_DR | UART_LSR_BI)) && (max_count-- > 0));
>> +     tty_flip_buffer_push(tty);
>> +}
>> +
>> +static void transmit_chars(struct uart_omap_port *up)
>> +{
>> +     struct circ_buf *xmit = &up->port.state->xmit;
>> +     int count;
>> +
>> +     if (up->port.x_char) {
>> +             serial_out(up, UART_TX, up->port.x_char);
>> +             up->port.icount.tx++;
>> +             up->port.x_char = 0;
>> +             return;
>> +     }
>> +     if (uart_circ_empty(xmit) || uart_tx_stopped(&up->port)) {
>> +             serial_omap_stop_tx(&up->port);
>> +             return;
>> +     }
>> +
>> +     count = up->port.fifosize / 4;
>> +     do {
>> +             serial_out(up, UART_TX, xmit->buf[xmit->tail]);
>> +             xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>> +             up->port.icount.tx++;
>> +             if (uart_circ_empty(xmit))
>> +                     break;
>> +     } while (--count > 0);
>> +
>> +     if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>> +             uart_write_wakeup(&up->port);
>> +
>> +     if (uart_circ_empty(xmit))
>> +             serial_omap_stop_tx(&up->port);
>> +}
>> +
>> +static void serial_omap_start_tx(struct uart_port *port)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +
>> +     if (up->use_dma && !(up->port.x_char)) {
>
> If you turn this test around you can lose one level of indentation and
> make the function more readable, by putting the 2-line else-case here
> followed by the return, and not indent the rest.
>
>
>> +
>> +             struct circ_buf *xmit = &up->port.state->xmit;
>> +             unsigned int start = up->uart_dma.tx_buf_dma_phys +
>> +                             (xmit->tail & (UART_XMIT_SIZE - 1));
>
> Might as well define these variables at the head of the function.
>
>> +             if (uart_circ_empty(xmit) || up->uart_dma.tx_dma_state)
>> +                     return;
>> +             spin_lock(&(up->uart_dma.tx_lock));
>> +             up->uart_dma.tx_dma_state = 1;
>
> What's state 1? Magic value.
>
>> +             spin_unlock(&(up->uart_dma.tx_lock));
>> +
>> +             up->uart_dma.tx_buf_size = uart_circ_chars_pending(xmit);
>> +             /*
>> +              * It is a circular buffer. See if the buffer has wounded back.
>> +              * If yes it will have to be transferred in two separate dma
>> +              * transfers
>> +              */
>> +             if (start + up->uart_dma.tx_buf_size >=
>> +                             up->uart_dma.tx_buf_dma_phys + UART_XMIT_SIZE)
>> +                     up->uart_dma.tx_buf_size =
>> +                             (up->uart_dma.tx_buf_dma_phys +
>> +                             UART_XMIT_SIZE) - start;
>> +
>> +             if (up->uart_dma.tx_dma_channel == 0xFF)
>> +                     omap_request_dma(up->uart_dma.uart_dma_tx,
>> +                                     "UART Tx DMA",
>> +                                     (void *)uart_tx_dma_callback, up,
>> +                                     &(up->uart_dma.tx_dma_channel));
>> +             omap_set_dma_dest_params(up->uart_dma.tx_dma_channel, 0,
>> +                                     OMAP_DMA_AMODE_CONSTANT,
>> +                                     up->uart_dma.uart_base, 0, 0);
>> +             omap_set_dma_src_params(up->uart_dma.tx_dma_channel, 0,
>> +                                     OMAP_DMA_AMODE_POST_INC, start, 0, 0);
>> +
>> +             omap_set_dma_transfer_params(up->uart_dma.tx_dma_channel,
>> +                                     OMAP_DMA_DATA_TYPE_S8,
>> +                                     up->uart_dma.tx_buf_size, 1,
>> +                                     OMAP_DMA_SYNC_ELEMENT,
>> +                                     up->uart_dma.uart_dma_tx, 0);
>> +
>> +             omap_start_dma(up->uart_dma.tx_dma_channel);
>> +
>> +     } else if (!(up->ier & UART_IER_THRI)) {
>> +             up->ier |= UART_IER_THRI;
>> +             serial_out(up, UART_IER, up->ier);
>> +     }
>> +}
>> +
>> +static unsigned int check_modem_status(struct uart_omap_port *up)
>> +{
>> +     int status;
>> +     status = serial_in(up, UART_MSR);
>> +
>> +     status |= up->msr_saved_flags;
>> +     up->msr_saved_flags = 0;
>> +
>> +     if ((status & UART_MSR_ANY_DELTA) == 0)
>> +             return status;
>> +     if (status & UART_MSR_ANY_DELTA && up->ier & UART_IER_MSI &&
>> +         up->port.state != NULL) {
>> +             if (status & UART_MSR_TERI)
>> +                     up->port.icount.rng++;
>> +             if (status & UART_MSR_DDSR)
>> +                     up->port.icount.dsr++;
>> +             if (status & UART_MSR_DDCD)
>> +                     uart_handle_dcd_change
>> +                             (&up->port, status & UART_MSR_DCD);
>> +             if (status & UART_MSR_DCTS)
>> +                     uart_handle_cts_change
>> +                             (&up->port, status & UART_MSR_CTS);
>> +             wake_up_interruptible(&up->port.state->port.delta_msr_wait);
>> +     }
>> +
>> +     return status;
>> +}
>> +
>> +/**
>> + * serial_omap_irq() - This handles the interrupt from one port
>> + * @irq: uart port irq number
>> + * @dev_id: uart port info
>> + */
>> +static inline irqreturn_t serial_omap_irq(int irq, void *dev_id)
>> +{
>> +     struct uart_omap_port *up = dev_id;
>> +     unsigned int iir, lsr;
>> +
>> +     iir = serial_in(up, UART_IIR);
>> +     if (iir & UART_IIR_NO_INT)
>> +             return IRQ_NONE;
>> +
>> +     lsr = serial_in(up, UART_LSR);
>> +     if ((iir & 0x4) && up->use_dma) {
>> +             up->ier &= ~UART_IER_RDI;
>> +             serial_out(up, UART_IER, up->ier);
>> +             serial_omap_start_rxdma(up);
>> +     } else if (lsr & UART_LSR_DR) {
>> +             receive_chars(up, &lsr);
>> +     }
>> +     check_modem_status(up);
>> +     if ((lsr & UART_LSR_THRE) && (iir & 0x2))
>> +             transmit_chars(up);
>> +
>> +     up->port_activity = jiffies;
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static unsigned int serial_omap_tx_empty(struct uart_port *port)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +     unsigned long flags = 0;
>> +     unsigned int ret;
>> +
>> +     dev_dbg(up->port.dev, "serial_omap_tx_empty+%d\n", up->pdev->id);
>> +     spin_lock_irqsave(&up->port.lock, flags);
>> +     ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
>> +     spin_unlock_irqrestore(&up->port.lock, flags);
>> +
>> +     return ret;
>> +}
>> +
>> +static unsigned int serial_omap_get_mctrl(struct uart_port *port)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +     unsigned char status;
>> +     unsigned int ret;
>> +
>> +     status = check_modem_status(up);
>> +     dev_dbg(up->port.dev, "serial_omap_get_mctrl+%d\n", up->pdev->id);
>> +
>> +     ret = 0;
>> +     if (status & UART_MSR_DCD)
>> +             ret |= TIOCM_CAR;
>> +     if (status & UART_MSR_RI)
>> +             ret |= TIOCM_RNG;
>> +     if (status & UART_MSR_DSR)
>> +             ret |= TIOCM_DSR;
>> +     if (status & UART_MSR_CTS)
>> +             ret |= TIOCM_CTS;
>> +     return ret;
>> +}
>> +
>> +static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +     unsigned char mcr = 0;
>> +
>> +     dev_dbg(up->port.dev, "serial_omap_set_mctrl+%d\n", up->pdev->id);
>> +     if (mctrl & TIOCM_RTS)
>> +             mcr |= UART_MCR_RTS;
>> +     if (mctrl & TIOCM_DTR)
>> +             mcr |= UART_MCR_DTR;
>> +     if (mctrl & TIOCM_OUT1)
>> +             mcr |= UART_MCR_OUT1;
>> +     if (mctrl & TIOCM_OUT2)
>> +             mcr |= UART_MCR_OUT2;
>> +     if (mctrl & TIOCM_LOOP)
>> +             mcr |= UART_MCR_LOOP;
>> +
>> +     mcr |= up->mcr;
>> +     serial_out(up, UART_MCR, mcr);
>> +}
>> +
>> +static void serial_omap_break_ctl(struct uart_port *port, int break_state)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +     unsigned long flags = 0;
>> +
>> +     dev_dbg(up->port.dev, "serial_omap_break_ctl+%d\n", up->pdev->id);
>> +     spin_lock_irqsave(&up->port.lock, flags);
>> +     if (break_state == -1)
>> +             up->lcr |= UART_LCR_SBC;
>> +     else
>> +             up->lcr &= ~UART_LCR_SBC;
>> +     serial_out(up, UART_LCR, up->lcr);
>> +     spin_unlock_irqrestore(&up->port.lock, flags);
>> +}
>> +
>> +static int serial_omap_startup(struct uart_port *port)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +     unsigned long flags = 0;
>> +     int retval;
>> +
>> +     /*
>> +      * Allocate the IRQ
>> +      */
>> +     retval = request_irq(up->port.irq, serial_omap_irq, up->port.irqflags,
>> +                             up->name, up);
>> +     if (retval)
>> +             return retval;
>> +
>> +     dev_dbg(up->port.dev, "serial_omap_startup+%d\n", up->pdev->id);
>> +
>> +     /*
>> +      * Clear the FIFO buffers and disable them.
>> +      * (they will be reenabled in set_termios())
>> +      */
>> +     serial_omap_clear_fifos(up);
>> +     /* For Hardware flow control */
>> +     serial_out(up, UART_MCR, 0x2);
>> +
>> +     /*
>> +      * Clear the interrupt registers.
>> +      */
>> +     (void) serial_in(up, UART_LSR);
>> +     if (serial_in(up, UART_LSR) & UART_LSR_DR)
>> +             (void) serial_in(up, UART_RX);
>> +     (void) serial_in(up, UART_IIR);
>> +     (void) serial_in(up, UART_MSR);
>> +
>> +     /*
>> +      * Now, initialize the UART
>> +      */
>> +     serial_out(up, UART_LCR, UART_LCR_WLEN8);
>> +     spin_lock_irqsave(&up->port.lock, flags);
>> +     if (up->port.flags & UPF_FOURPORT) {
>> +             if (!is_real_interrupt(up->port.irq))
>> +                     up->port.mctrl |= TIOCM_OUT1;
>> +     } else {
>> +             /*
>> +              * Most PC uarts need OUT2 raised to enable interrupts.
>> +              */
>> +             if (is_real_interrupt(up->port.irq))
>> +                     up->port.mctrl |= TIOCM_OUT2;
>> +     }
>
> The above if/else seems to come straight over from the 8250 driver. One
> of the cases can likely be removed here. :)
>
>> +     serial_omap_set_mctrl(&up->port, up->port.mctrl);
>> +     spin_unlock_irqrestore(&up->port.lock, flags);
>> +
>> +     up->msr_saved_flags = 0;
>> +
>> +     if (up->use_dma) {
>> +             if (!up->is_buf_dma_alloced) {
>
> Will you ever come here with use_dma = 1 and is_buf_dma_alloced = 0?
>
> I.e. do yo need the separate variable at all?
>
>> +                     free_page((unsigned long)up->port.state->xmit.buf);
>> +                     up->port.state->xmit.buf = NULL;
>> +                     up->port.state->xmit.buf = dma_alloc_coherent(NULL,
>> +                             UART_XMIT_SIZE,
>> +                             (dma_addr_t *)&(up->uart_dma.tx_buf_dma_phys),
>> +                             0);
>
> Huh, why two assignments in a row?
>
>> +                     up->is_buf_dma_alloced = 1;
>> +             }
>> +             init_timer(&(up->uart_dma.rx_timer));
>> +             up->uart_dma.rx_timer.function = serial_omap_rx_timeout;
>> +             up->uart_dma.rx_timer.data = up->pdev->id;
>> +             /* Currently the buffer size is 4KB. Can increase it */
>> +             up->uart_dma.rx_buf = dma_alloc_coherent(NULL,
>> +                     up->uart_dma.rx_buf_size,
>> +                     (dma_addr_t *)&(up->uart_dma.rx_buf_dma_phys), 0);
>> +     }
>> +
>> +     /*
>> +      * Finally, enable interrupts. Note: Modem status interrupts
>> +      * are set via set_termios(), which will be occurring imminently
>> +      * anyway, so we don't enable them here.
>> +      */
>> +     up->ier = UART_IER_RLSI | UART_IER_RDI;
>> +     serial_out(up, UART_IER, up->ier);
>> +
>> +     up->port_activity = jiffies;
>> +     return 0;
>> +}
>> +
>> +static void serial_omap_shutdown(struct uart_port *port)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +     unsigned long flags = 0;
>> +
>> +     dev_dbg(up->port.dev, "serial_omap_shutdown+%d\n", up->pdev->id);
>> +     /*
>> +      * Disable interrupts from this port
>> +      */
>> +     up->ier = 0;
>> +     serial_out(up, UART_IER, 0);
>> +
>> +     spin_lock_irqsave(&up->port.lock, flags);
>> +     if (up->port.flags & UPF_FOURPORT) {
>> +             /* reset interrupts on the AST Fourport board */
>> +             inb((up->port.iobase & 0xfe0) | 0x1f);
>> +             up->port.mctrl |= TIOCM_OUT1;
>> +     } else
>> +             up->port.mctrl &= ~TIOCM_OUT2;
>
> Same comment here regarding fourport and 8250
>
>> +     serial_omap_set_mctrl(&up->port, up->port.mctrl);
>> +     spin_unlock_irqrestore(&up->port.lock, flags);
>> +
>> +     /*
>> +      * Disable break condition and FIFOs
>> +      */
>> +     serial_out(up, UART_LCR, serial_in(up, UART_LCR) & ~UART_LCR_SBC);
>> +     serial_omap_clear_fifos(up);
>> +
>> +     /*
>> +      * Read data port to reset things, and then free the irq
>> +      */
>> +     if (serial_in(up, UART_LSR) & UART_LSR_DR)
>> +             (void) serial_in(up, UART_RX);
>> +     if (up->use_dma) {
>> +             int tmp;
>> +             if (up->is_buf_dma_alloced) {
>
> ... same comment as above.
>
>> +                     dma_free_coherent(up->port.dev,
>> +                             UART_XMIT_SIZE,
>> +                             up->port.state->xmit.buf,
>> +                             up->uart_dma.tx_buf_dma_phys);
>> +                     up->port.state->xmit.buf = NULL;
>> +                     up->is_buf_dma_alloced = 0;
>> +             }
>> +             serial_omap_stop_rx(port);
>> +             dma_free_coherent(up->port.dev,
>> +                             up->uart_dma.rx_buf_size,
>> +                             up->uart_dma.rx_buf,
>> +                             up->uart_dma.rx_buf_dma_phys);
>> +             up->uart_dma.rx_buf = NULL;
>> +             tmp = serial_in(up, UART_OMAP_SYSC) & 0x7;
>> +             serial_out(up, UART_OMAP_SYSC, tmp); /* force-idle */
>> +     }
>> +     free_irq(up->port.irq, up);
>> +}
>> +
>> +static inline void
>> +serial_omap_configure_xonxoff
>> +             (struct uart_omap_port *up, struct ktermios *termios)
>> +{
>> +     unsigned char efr = 0;
>> +
>> +     up->lcr = serial_in(up, UART_LCR);
>> +     serial_out(up, UART_LCR, 0xbf);
>> +     up->efr = serial_in(up, UART_EFR);
>> +     serial_out(up, UART_EFR, up->efr & ~UART_EFR_ECB);
>> +
>> +     serial_out(up, UART_XON1, termios->c_cc[VSTART]);
>> +     serial_out(up, UART_XOFF1, termios->c_cc[VSTOP]);
>> +
>> +     /* clear SW control mode bits */
>> +     efr = up->efr;
>> +     efr &= 0xf0;
>> +
>> +     /*
>> +      * IXON Flag:
>> +      * Enable XON/XOFF flow control on output.
>> +      * Transmit XON1, XOFF1
>> +      */
>> +     if (termios->c_iflag & IXON)
>> +             efr |= 0x01 << 3;
>> +
>> +     /*
>> +      * IXOFF Flag:
>> +      * Enable XON/XOFF flow control on input.
>> +      * Receiver compares XON1, XOFF1.
>> +      */
>> +     if (termios->c_iflag & IXOFF)
>> +             efr |= 0x01 << 1;
>> +
>> +     serial_out(up, UART_EFR, up->efr | UART_EFR_ECB);
>
> So, UART_EFR_ECB is documented, but it would be nice to have the other
> bits documented as #defines too.
>
>> +     serial_out(up, UART_LCR, 0x80);
>> +
>> +     up->mcr = serial_in(up, UART_MCR);
>> +
>> +     /*
>> +      * IXANY Flag:
>> +      * Enable any character to restart output.
>> +      * Operation resumes after receiving any
>> +      * character after recognition of the XOFF character
>> +      */
>> +     if (termios->c_iflag & IXANY)
>> +             up->mcr |= 1<<5;
>> +
>> +     serial_out(up, UART_MCR, up->mcr | 1<<6);
>> +
>> +     serial_out(up, UART_LCR, 0xbf);
>> +     serial_out(up, UART_TI752_TCR, 0x0f);
>> +     /* Enable special char function UARTi.EFR_REG[5] and
>> +      * load the new software flow control mode IXON or IXOFF
>> +      * and restore the UARTi.EFR_REG[4] ENHANCED_EN value.
>> +      */
>> +     serial_out(up, UART_EFR, efr | 1<<5);
>> +     serial_out(up, UART_LCR, 0x80);
>> +
>> +     serial_out(up, UART_MCR, up->mcr & ~(1<<6));
>> +     serial_out(up, UART_LCR, up->lcr);
>> +}
>> +
>> +static void
>> +serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>> +                     struct ktermios *old)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +     unsigned char cval = 0;
>> +     unsigned char efr = 0;
>> +     unsigned long flags = 0;
>> +     unsigned int baud, quot;
>> +
>> +     switch (termios->c_cflag & CSIZE) {
>> +     case CS5:
>> +             cval = UART_LCR_WLEN5;
>> +             break;
>> +     case CS6:
>> +             cval = UART_LCR_WLEN6;
>> +             break;
>> +     case CS7:
>> +             cval = UART_LCR_WLEN7;
>> +             break;
>> +     default:
>> +     case CS8:
>> +             cval = UART_LCR_WLEN8;
>> +             break;
>> +     }
>> +
>> +     if (termios->c_cflag & CSTOPB)
>> +             cval |= UART_LCR_STOP;
>> +     if (termios->c_cflag & PARENB)
>> +             cval |= UART_LCR_PARITY;
>> +     if (!(termios->c_cflag & PARODD))
>> +             cval |= UART_LCR_EPAR;
>> +
>> +     /*
>> +      * Ask the core to calculate the divisor for us.
>> +      */
>> +
>> +     baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/13);
>> +     quot = serial_omap_get_divisor(port, baud);
>> +
>> +     if (up->use_dma)
>> +             up->fcr = UART_FCR_ENABLE_FIFO
>> +                                     | 0x1 << 6 | 0x1 << 4
>> +                                     | UART_FCR_DMA_SELECT;
>> +     else
>> +             up->fcr = UART_FCR_ENABLE_FIFO
>> +                                     | 0x1 << 6 | 0x1 << 4;
>
> More magic numbers.
>
>> +
>> +     /*
>> +      * Ok, we're now changing the port state.  Do it with
>> +      * interrupts disabled.
>> +      */
>> +     spin_lock_irqsave(&up->port.lock, flags);
>> +
>> +     /*
>> +      * Update the per-port timeout.
>> +      */
>> +     uart_update_timeout(port, termios->c_cflag, baud);
>> +
>> +     up->port.read_status_mask = UART_LSR_OE | UART_LSR_THRE | UART_LSR_DR;
>> +     if (termios->c_iflag & INPCK)
>> +             up->port.read_status_mask |= UART_LSR_FE | UART_LSR_PE;
>> +     if (termios->c_iflag & (BRKINT | PARMRK))
>> +             up->port.read_status_mask |= UART_LSR_BI;
>> +
>> +     /*
>> +      * Characters to ignore
>> +      */
>> +     up->port.ignore_status_mask = 0;
>> +     if (termios->c_iflag & IGNPAR)
>> +             up->port.ignore_status_mask |= UART_LSR_PE | UART_LSR_FE;
>> +     if (termios->c_iflag & IGNBRK) {
>> +             up->port.ignore_status_mask |= UART_LSR_BI;
>> +             /*
>> +              * If we're ignoring parity and break indicators,
>> +              * ignore overruns too (for real raw support).
>> +              */
>> +             if (termios->c_iflag & IGNPAR)
>> +                     up->port.ignore_status_mask |= UART_LSR_OE;
>> +     }
>> +
>> +     /*
>> +      * ignore all characters if CREAD is not set
>> +      */
>> +     if ((termios->c_cflag & CREAD) == 0)
>> +             up->port.ignore_status_mask |= UART_LSR_DR;
>> +
>> +     /*
>> +      * Modem status interrupts
>> +      */
>> +     up->ier &= ~UART_IER_MSI;
>> +     if (UART_ENABLE_MS(&up->port, termios->c_cflag))
>> +             up->ier |= UART_IER_MSI;
>> +     serial_out(up, UART_IER, up->ier);
>> +     serial_out(up, UART_LCR, cval);         /* reset DLAB */
>> +
>> +     /*-----------FIFOs and DMA Settings -----------*/
>
> Unstandard comment style
>
>> +
>> +     /* FCR can be changed only when the
>> +      * baud clock is not running
>> +      * DLL_REG and DLH_REG set to 0.
>> +      */
>> +     serial_out(up, UART_LCR, UART_LCR_DLAB);
>> +     serial_out(up, UART_DLL, 0);
>> +     serial_out(up, UART_DLM, 0);
>> +     serial_out(up, UART_LCR, 0);
>> +
>> +     serial_out(up, UART_LCR, 0xbf);
>> +
>> +     up->efr = serial_in(up, UART_EFR);
>> +     serial_out(up, UART_EFR, up->efr | UART_EFR_ECB);
>> +     serial_out(up, UART_LCR, 0x0);          /* Access FCR */
>
> Access FCR by writing LCR? Please explain. Same goes for the ones below.

Most of these configuration come straight from TRM:

7.5 UART/IrDA/CIR Basic Programming Model

>
>> +
>> +     up->mcr = serial_in(up, UART_MCR);
>> +     serial_out(up, UART_MCR, up->mcr | 0x40);       /* Access TLR*/
>> +     /* FIFO ENABLE, DMA MODE */
>> +     serial_out(up, UART_FCR, up->fcr);
>> +     serial_out(up, UART_LCR, 0xbf);         /* Access EFR */
>> +
>> +     if (up->use_dma) {
>> +             serial_out(up, UART_TI752_TLR, 0x00);
>> +             serial_out(up, UART_OMAP_SCR, ((1 << 6) | (1 << 7)));
>> +     }
>> +
>> +     serial_out(up, UART_EFR, up->efr);
>> +     serial_out(up, UART_LCR, 0x80);
>> +     serial_out(up, UART_MCR, up->mcr);      /* Restore TLR */
>> +
>> +     /*-----Protocol, Baud Rate, and Interrupt Settings -- */
>
> More unstandard comment format
>
>> +
>> +     serial_out(up, UART_OMAP_MDR1, OMAP_MDR1_DISABLE);
>> +
>> +     serial_out(up, UART_LCR, 0xbf);
>> +
>> +     up->efr = serial_in(up, UART_EFR);
>> +     serial_out(up, UART_EFR, up->efr | UART_EFR_ECB);
>> +
>> +     serial_out(up, UART_LCR, 0);
>> +     serial_out(up, UART_IER, 0);
>> +     serial_out(up, UART_LCR, 0xbf);
>> +
>> +     serial_out(up, UART_DLL, quot & 0xff);          /* LS of divisor */
>> +     serial_out(up, UART_DLM, quot >> 8);            /* MS of divisor */
>> +
>> +     serial_out(up, UART_LCR, 0);
>> +     serial_out(up, UART_IER, up->ier);
>> +     serial_out(up, UART_LCR, 0xbf);
>> +
>> +     serial_out(up, UART_EFR, up->efr);
>> +     serial_out(up, UART_LCR, cval);
>> +
>> +     if (baud > 230400 && baud != 3000000)
>> +             serial_out(up, UART_OMAP_MDR1, OMAP_MDR1_MODE13X);
>> +     else
>> +             serial_out(up, UART_OMAP_MDR1, OMAP_MDR1_MODE16X);
>
> More 13-divider special case.
>
>> +
>> +     /*---------- Hardware Flow Control Configuration------- */
>> +
>> +     if (termios->c_cflag & CRTSCTS) {
>> +             efr |= (UART_EFR_CTS | UART_EFR_RTS);
>> +             serial_out(up, UART_LCR, 0x80);
>> +
>> +             up->mcr = serial_in(up, UART_MCR);
>> +             serial_out(up, UART_MCR, up->mcr | 0x40);
>> +
>> +             serial_out(up, UART_LCR, 0xbf); /* Access EFR */
>
> EFR or LCR?!

writing 0xbf to LCR gives acces to EFR

references : 17.5 UART/IrDA/CIR Basic Programming Model

>
>> +
>> +             up->efr = serial_in(up, UART_EFR);
>> +             serial_out(up, UART_EFR, up->efr | UART_EFR_ECB);
>> +
>> +             serial_out(up, UART_TI752_TCR, 0x0f);
>> +             serial_out(up, UART_EFR, efr); /* Enable AUTORTS and AUTOCTS */
>> +             serial_out(up, UART_LCR, 0x80);
>> +             serial_out(up, UART_MCR, up->mcr | 0x02);
>> +             serial_out(up, UART_LCR, cval);
>> +     }
>> +
>> +     serial_omap_set_mctrl(&up->port, up->port.mctrl);
>> +     /* --------Software Flow Control Configuration-------- */
>> +     if (termios->c_iflag & (IXON | IXOFF))
>> +             serial_omap_configure_xonxoff(up, termios);
>> +
>> +     spin_unlock_irqrestore(&up->port.lock, flags);
>> +     dev_dbg(up->port.dev, "serial_omap_set_termios+%d\n", up->pdev->id);
>> +}
>> +
>> +static void
>> +serial_omap_pm(struct uart_port *port, unsigned int state,
>> +            unsigned int oldstate)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +     unsigned char efr;
>> +
>> +     dev_dbg(up->port.dev, "serial_omap_pm+%d\n", up->pdev->id);
>> +     efr = serial_in(up, UART_EFR);
>> +     serial_out(up, UART_LCR, 0xBF);
>> +     serial_out(up, UART_EFR, efr | UART_EFR_ECB);
>> +     serial_out(up, UART_LCR, 0);
>> +
>> +     serial_out(up, UART_IER, (state != 0) ? UART_IERX_SLEEP : 0);
>> +     serial_out(up, UART_LCR, 0xBF);
>> +     serial_out(up, UART_EFR, efr);
>> +     serial_out(up, UART_LCR, 0);
>> +     /* Enable module level wake up */
>> +     serial_out(up, UART_OMAP_WER, (state != 0) ? 0x7f : 0);
>> +}
>> +
>> +static void serial_omap_release_port(struct uart_port *port)
>> +{
>> +     dev_dbg(port->dev, "serial_omap_release_port+\n");
>> +}
>> +
>> +static int serial_omap_request_port(struct uart_port *port)
>> +{
>> +     dev_dbg(port->dev, "serial_omap_request_port+\n");
>> +     return 0;
>> +}
>> +
>> +static void serial_omap_config_port(struct uart_port *port, int flags)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +
>> +     dev_dbg(up->port.dev, "serial_omap_config_port+%d\n",
>> +                                                     up->pdev->id);
>> +     up->port.type = PORT_OMAP;
>> +}
>> +
>> +static int
>> +serial_omap_verify_port(struct uart_port *port, struct serial_struct *ser)
>> +{
>> +     /* we don't want the core code to modify any port params */
>> +     dev_dbg(port->dev, "serial_omap_verify_port+\n");
>> +     return -EINVAL;
>> +}
>> +
>> +static const char *
>> +serial_omap_type(struct uart_port *port)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +
>> +     dev_dbg(up->port.dev, "serial_omap_type+%d\n", up->pdev->id);
>> +     return up->name;
>> +}
>> +
>> +#ifdef CONFIG_SERIAL_OMAP_CONSOLE
>> +
>> +static struct uart_omap_port *serial_omap_console_ports[4];
>> +
>> +static struct uart_driver serial_omap_reg;
>> +
>> +#define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
>> +
>> +static inline void wait_for_xmitr(struct uart_omap_port *up)
>> +{
>> +     unsigned int status, tmout = 10000;
>> +
>> +     /* Wait up to 10ms for the character(s) to be sent. */
>> +     do {
>> +             status = serial_in(up, UART_LSR);
>> +
>> +             if (status & UART_LSR_BI)
>> +                     up->lsr_break_flag = UART_LSR_BI;
>> +
>> +             if (--tmout == 0)
>> +                     break;
>> +             udelay(1);
>> +     } while ((status & BOTH_EMPTY) != BOTH_EMPTY);
>> +
>> +     /* Wait up to 1s for flow control if necessary */
>> +     if (up->port.flags & UPF_CONS_FLOW) {
>> +             tmout = 1000000;
>> +             for (tmout = 1000000; tmout; tmout--) {
>> +                     unsigned int msr = serial_in(up, UART_MSR);
>> +                     up->msr_saved_flags |= msr & MSR_SAVE_FLAGS;
>> +                     if (msr & UART_MSR_CTS)
>> +                             break;
>> +                     udelay(1);
>> +             }
>> +     }
>> +}
>> +
>> +static void serial_omap_console_putchar(struct uart_port *port, int ch)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +
>> +     wait_for_xmitr(up);
>> +     serial_out(up, UART_TX, ch);
>> +}
>> +
>> +static void
>> +serial_omap_console_write(struct console *co, const char *s,
>> +             unsigned int count)
>> +{
>> +     struct uart_omap_port *up = serial_omap_console_ports[co->index];
>> +     unsigned int ier;
>> +
>> +     /*
>> +      * First save the IER then disable the interrupts
>> +      */
>> +     ier = serial_in(up, UART_IER);
>> +     serial_out(up, UART_IER, 0);
>> +
>> +     uart_console_write(&up->port, s, count, serial_omap_console_putchar);
>> +
>> +     /*
>> +      * Finally, wait for transmitter to become empty
>> +      * and restore the IER
>> +      */
>> +     wait_for_xmitr(up);
>> +     serial_out(up, UART_IER, ier);
>> +     /*
>> +      * The receive handling will happen properly because the
>> +      * receive ready bit will still be set; it is not cleared
>> +      * on read.  However, modem control will not, we must
>> +      * call it if we have saved something in the saved flags
>> +      * while processing with interrupts off.
>> +      */
>> +     if (up->msr_saved_flags)
>> +             check_modem_status(up);
>> +}
>> +
>> +static int __init
>> +serial_omap_console_setup(struct console *co, char *options)
>> +{
>> +     struct uart_omap_port *up;
>> +     int baud = 9600;
>
> Nearly every OMAP setup seems to prefer 115200 as default baudrate, right?

Yes, but shouldn't we initialize to lowest possible baud?

>
>> +     int bits = 8;
>> +     int parity = 'n';
>> +     int flow = 'n';
>> +     int r;
>> +
>> +     if (serial_omap_console_ports[co->index] == NULL)
>> +             return -ENODEV;
>> +     up = serial_omap_console_ports[co->index];
>> +
>> +     if (options)
>> +             uart_parse_options(options, &baud, &parity, &bits, &flow);
>> +
>> +     r = uart_set_options(&up->port, co, baud, parity, bits, flow);
>> +
>> +     return r;
>> +}
>> +
>> +static struct console serial_omap_console = {
>> +     .name           = OMAP_SERIAL_NAME,
>> +     .write          = serial_omap_console_write,
>> +     .device         = uart_console_device,
>> +     .setup          = serial_omap_console_setup,
>> +     .flags          = CON_PRINTBUFFER,
>> +     .index          = -1,
>> +     .data           = &serial_omap_reg,
>> +};
>> +
>> +static void serial_omap_add_console_port(struct uart_omap_port *up)
>> +{
>> +     serial_omap_console_ports[up->pdev->id] = up;
>> +}
>> +
>> +#define OMAP_CONSOLE (&serial_omap_console)
>> +
>> +#else
>> +
>> +#define OMAP_CONSOLE NULL
>> +
>> +static inline void serial_omap_add_console_port(struct uart_omap_port *up) {}
>
> Please do the empty body on a separate line.
>
>> +
>> +#endif
>> +
>> +struct uart_ops serial_omap_pops = {
>> +     .tx_empty       = serial_omap_tx_empty,
>> +     .set_mctrl      = serial_omap_set_mctrl,
>> +     .get_mctrl      = serial_omap_get_mctrl,
>> +     .stop_tx        = serial_omap_stop_tx,
>> +     .start_tx       = serial_omap_start_tx,
>> +     .stop_rx        = serial_omap_stop_rx,
>> +     .enable_ms      = serial_omap_enable_ms,
>> +     .break_ctl      = serial_omap_break_ctl,
>> +     .startup        = serial_omap_startup,
>> +     .shutdown       = serial_omap_shutdown,
>> +     .set_termios    = serial_omap_set_termios,
>> +     .pm             = serial_omap_pm,
>> +     .type           = serial_omap_type,
>> +     .release_port   = serial_omap_release_port,
>> +     .request_port   = serial_omap_request_port,
>> +     .config_port    = serial_omap_config_port,
>> +     .verify_port    = serial_omap_verify_port,
>> +};
>> +
>> +static struct uart_driver serial_omap_reg = {
>> +     .owner          = THIS_MODULE,
>> +     .driver_name    = "OMAP-SERIAL",
>> +     .dev_name       = OMAP_SERIAL_NAME,
>> +     .major          = OMAP_SERIAL_MAJOR,
>> +     .minor          = OMAP_SERIAL_MINOR,
>> +     .nr             = OMAP_MAX_HSUART_PORTS,
>> +     .cons           = OMAP_CONSOLE,
>> +};
>> +
>> +static
>> +int serial_omap_suspend(struct platform_device *pdev, pm_message_t state)
>
> No linewrap (checkpatch should have caught this)

I use checkpatch --strict,
don't know how this passed

>
>> +{
>> +     struct uart_omap_port *up = platform_get_drvdata(pdev);
>> +
>> +     if (up)
>> +             uart_suspend_port(&serial_omap_reg, &up->port);
>> +     return 0;
>> +}
>> +
>> +static int serial_omap_resume(struct platform_device *dev)
>> +{
>> +     struct uart_omap_port *up = platform_get_drvdata(dev);
>> +
>> +     if (up)
>> +             uart_resume_port(&serial_omap_reg, &up->port);
>> +     return 0;
>> +}
>> +
>> +static void serial_omap_rx_timeout(unsigned long uart_no)
>> +{
>> +     struct uart_omap_port *up = ui[uart_no];
>> +     unsigned int curr_dma_pos;
>> +     curr_dma_pos = omap_get_dma_dst_pos(up->uart_dma.rx_dma_channel);
>> +
>> +     if ((curr_dma_pos == up->uart_dma.prev_rx_dma_pos) ||
>> +                          (curr_dma_pos == 0)) {
>> +             if (jiffies_to_msecs(jiffies - up->port_activity) <
>> +                                                     RX_TIMEOUT) {
>> +                     mod_timer(&up->uart_dma.rx_timer, jiffies +
>> +                             usecs_to_jiffies(up->uart_dma.rx_timeout));
>> +             } else {
>> +                     serial_omap_stop_rxdma(up);
>> +                     up->ier |= UART_IER_RDI;
>> +                     serial_out(up, UART_IER, up->ier);
>> +             }
>> +             return;
>> +     } else {
>
> Since you have a return above, there's no need to do the rest in the
> else-clause. Saves one level of indentation.
>
>> +             unsigned int curr_transmitted_size = curr_dma_pos -
>> +                                             up->uart_dma.prev_rx_dma_pos;
>> +
>> +             up->port.icount.rx += curr_transmitted_size;
>> +             tty_insert_flip_string(up->port.state->port.tty,
>> +                             up->uart_dma.rx_buf +
>> +                             (up->uart_dma.prev_rx_dma_pos -
>> +                             up->uart_dma.rx_buf_dma_phys),
>> +                             curr_transmitted_size);
>> +             tty_flip_buffer_push(up->port.state->port.tty);
>> +             up->uart_dma.prev_rx_dma_pos = curr_dma_pos;
>> +             if (up->uart_dma.rx_buf_size +
>> +                             up->uart_dma.rx_buf_dma_phys == curr_dma_pos)
>> +                     serial_omap_start_rxdma(up);
>> +             else
>> +                     mod_timer(&up->uart_dma.rx_timer, jiffies +
>> +                             usecs_to_jiffies(up->uart_dma.rx_timeout));
>> +             up->port_activity = jiffies;
>> +     }
>> +}
>> +
>> +static void uart_rx_dma_callback(int lch, u16 ch_status, void *data)
>> +{
>> +     return;
>> +}
>> +
>> +static void serial_omap_start_rxdma(struct uart_omap_port *up)
>> +{
>> +     if (up->uart_dma.rx_dma_channel == 0xFF) {
>> +             omap_request_dma(up->uart_dma.uart_dma_rx, "UART Rx DMA",
>> +                     (void *)uart_rx_dma_callback, up,
>> +                             &(up->uart_dma.rx_dma_channel));
>> +
>> +             omap_set_dma_src_params(up->uart_dma.rx_dma_channel, 0,
>> +                             OMAP_DMA_AMODE_CONSTANT,
>> +                             up->uart_dma.uart_base, 0, 0);
>> +             omap_set_dma_dest_params(up->uart_dma.rx_dma_channel, 0,
>> +                             OMAP_DMA_AMODE_POST_INC,
>> +                             up->uart_dma.rx_buf_dma_phys, 0, 0);
>> +             omap_set_dma_transfer_params(up->uart_dma.rx_dma_channel,
>> +                             OMAP_DMA_DATA_TYPE_S8,
>> +                             up->uart_dma.rx_buf_size, 1,
>> +                             OMAP_DMA_SYNC_ELEMENT,
>> +                             up->uart_dma.uart_dma_rx, 0);
>> +     }
>> +     up->uart_dma.prev_rx_dma_pos = up->uart_dma.rx_buf_dma_phys;
>> +     if (cpu_is_omap44xx())
>> +             omap_writel(0, OMAP44XX_DMA4_BASE
>> +                     + OMAP_DMA4_CDAC(up->uart_dma.rx_dma_channel));
>> +     else
>> +             omap_writel(0, OMAP34XX_DMA4_BASE
>> +                     + OMAP_DMA4_CDAC(up->uart_dma.rx_dma_channel));
>> +     omap_start_dma(up->uart_dma.rx_dma_channel);
>> +     mod_timer(&up->uart_dma.rx_timer, jiffies +
>> +                             usecs_to_jiffies(up->uart_dma.rx_timeout));
>> +     up->uart_dma.rx_dma_state = 1;
>> +}
>> +
>> +static void serial_omap_continue_tx(struct uart_omap_port *up)
>> +{
>> +     struct circ_buf *xmit = &up->port.state->xmit;
>> +     int start = up->uart_dma.tx_buf_dma_phys
>> +                     + (xmit->tail & (UART_XMIT_SIZE - 1));
>> +
>> +     if (uart_circ_empty(xmit))
>> +             return;
>> +
>> +     up->uart_dma.tx_buf_size = uart_circ_chars_pending(xmit);
>> +     /*
>> +      * It is a circular buffer. See if the buffer has wounded back.
>> +      * If yes it will have to be transferred in two separate dma
>> +      * transfers
>> +      */
>> +     if (start + up->uart_dma.tx_buf_size >=
>> +                     up->uart_dma.tx_buf_dma_phys + UART_XMIT_SIZE)
>> +             up->uart_dma.tx_buf_size =
>> +                     (up->uart_dma.tx_buf_dma_phys + UART_XMIT_SIZE) - start;
>> +     omap_set_dma_dest_params(up->uart_dma.tx_dma_channel, 0,
>> +                             OMAP_DMA_AMODE_CONSTANT,
>> +                             up->uart_dma.uart_base, 0, 0);
>> +     omap_set_dma_src_params(up->uart_dma.tx_dma_channel, 0,
>> +                             OMAP_DMA_AMODE_POST_INC, start, 0, 0);
>> +     omap_set_dma_transfer_params(up->uart_dma.tx_dma_channel,
>> +                             OMAP_DMA_DATA_TYPE_S8,
>> +                             up->uart_dma.tx_buf_size, 1,
>> +                             OMAP_DMA_SYNC_ELEMENT,
>> +                             up->uart_dma.uart_dma_tx, 0);
>
>
> Isn't there a dma_sync call missing somewhere around here? Same for
> the similar RX setup (both before DMA is started, as well as once it is
> completed on the RX side).

Sorry i didn't get you here as don't see any such dma_sync call
in arch/arm/plat-omap/dma.c.

>
>> +
>> +     omap_start_dma(up->uart_dma.tx_dma_channel);
>> +}
>> +
>> +static void uart_tx_dma_callback(int lch, u16 ch_status, void *data)
>> +{
>> +     struct uart_omap_port *up = (struct uart_omap_port *)data;
>> +     struct circ_buf *xmit = &up->port.state->xmit;
>> +
>> +     xmit->tail = (xmit->tail + up->uart_dma.tx_buf_size) & \
>> +                     (UART_XMIT_SIZE - 1);
>> +     up->port.icount.tx += up->uart_dma.tx_buf_size;
>> +
>> +     if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>> +             uart_write_wakeup(&up->port);
>> +
>> +     if (uart_circ_empty(xmit)) {
>> +             spin_lock(&(up->uart_dma.tx_lock));
>> +             serial_omap_stop_tx(&up->port);
>> +             up->uart_dma.tx_dma_state = 0;
>> +             spin_unlock(&(up->uart_dma.tx_lock));
>> +     } else {
>> +             omap_stop_dma(up->uart_dma.tx_dma_channel);
>> +             serial_omap_continue_tx(up);
>> +     }
>> +     up->port_activity = jiffies;
>> +     return;
>> +}
>> +
>> +static int serial_omap_probe(struct platform_device *pdev)
>> +{
>> +     struct uart_omap_port   *up;
>> +     struct resource         *mem, *irq, *dma_tx, *dma_rx;
>> +     struct omap_uart_port_info *omap_up_info = pdev->dev.platform_data;
>> +     int ret = -ENOSPC;
>> +     char str[7];
>> +
>> +     mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     if (!mem) {
>> +             dev_err(&pdev->dev, "no mem resource?\n");
>> +             return -ENODEV;
>> +     }
>> +     irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> +     if (!irq) {
>> +             dev_err(&pdev->dev, "no irq resource?\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     ret = (int) request_mem_region(mem->start, (mem->end - mem->start) + 1,
>> +                                  pdev->dev.driver->name);
>> +     if (!ret) {
>> +             dev_err(&pdev->dev, "memory region already claimed\n");
>> +             return -EBUSY;
>> +     }
>> +
>> +     dma_tx = platform_get_resource(pdev, IORESOURCE_DMA, 0);
>> +     if (!dma_tx) {
>> +             ret = -EINVAL;
>> +             goto err;
>> +     }
>> +
>> +     dma_rx = platform_get_resource(pdev, IORESOURCE_DMA, 1);
>> +     if (!dma_rx) {
>> +             ret = -EINVAL;
>> +             goto err;
>> +     }
>> +
>> +     up = kzalloc(sizeof(*up), GFP_KERNEL);
>> +     if (up == NULL) {
>> +             ret = -ENOMEM;
>> +             goto do_release_region;
>> +     }
>> +     sprintf(up->name, "OMAP UART%d", pdev->id);
>> +     up->pdev = pdev;
>> +     up->port.dev = &pdev->dev;
>> +     up->port.type = PORT_OMAP;
>> +     up->port.iotype = UPIO_MEM;
>> +     up->port.irq = irq->start;
>> +     up->port.regshift = 2;
>> +     up->port.fifosize = 64;
>> +     up->port.ops = &serial_omap_pops;
>> +     up->port.line = pdev->id;
>> +
>> +     up->port.membase = omap_up_info->membase;
>> +     up->port.mapbase = omap_up_info->mapbase;
>> +     up->port.flags = omap_up_info->flags;
>> +     up->port.irqflags = omap_up_info->irqflags;
>> +     up->port.uartclk = omap_up_info->uartclk;
>> +     up->uart_dma.uart_base = mem->start;
>> +     if (omap_up_info->dma_enabled) {
>> +             up->uart_dma.uart_dma_tx = dma_tx->start;
>> +             up->uart_dma.uart_dma_rx = dma_rx->start;
>> +             up->use_dma = 1;
>> +             up->uart_dma.rx_buf_size = 4096;
>> +             up->uart_dma.rx_timeout = 1;
>> +     }
>> +
>> +     if (up->use_dma) {
>> +             spin_lock_init(&(up->uart_dma.tx_lock));
>> +             spin_lock_init(&(up->uart_dma.rx_lock));
>> +             up->uart_dma.tx_dma_channel = 0xFF;
>> +             up->uart_dma.rx_dma_channel = 0xFF;
>> +     }
>> +     if (console_detect(str)) {
>> +             pr_err("\n %s: Invalid console paramter...\n", __func__);
>> +             pr_err("\n %s: UART Driver Init Failed!\n", __func__);
>> +             return -EPERM;
>> +     }
>
> See comments above about console_detect()
>
>> +     ui[pdev->id] = up;
>> +     serial_omap_add_console_port(up);
>> +
>> +     ret = uart_add_one_port(&serial_omap_reg, &up->port);
>> +     if (ret != 0)
>> +             goto do_release_region;
>> +
>> +     platform_set_drvdata(pdev, up);
>> +     return 0;
>> +err:
>> +     dev_err(&pdev->dev, "[UART%d]: failure [%s]\n",
>> +                                     pdev->id, __func__);
>
> It would be nice to get the actual error (i.e. printing out err) here.
>
>> +do_release_region:
>> +     release_mem_region(mem->start, (mem->end - mem->start) + 1);
>> +     return ret;
>> +}
>> +
>> +static int serial_omap_remove(struct platform_device *dev)
>> +{
>> +     struct uart_omap_port *up = platform_get_drvdata(dev);
>> +
>> +     platform_set_drvdata(dev, NULL);
>> +     if (up) {
>> +             uart_remove_one_port(&serial_omap_reg, &up->port);
>> +             kfree(up);
>> +     }
>> +     return 0;
>> +}
>> +
>> +static struct platform_driver serial_omap_driver = {
>> +     .probe          = serial_omap_probe,
>> +     .remove         = serial_omap_remove,
>> +
>> +     .suspend        = serial_omap_suspend,
>> +     .resume         = serial_omap_resume,
>> +     .driver         = {
>> +             .name   = DRIVER_NAME,
>> +     },
>> +};
>> +
>> +int __init serial_omap_init(void)
>> +{
>> +     int ret;
>> +
>> +     ret = uart_register_driver(&serial_omap_reg);
>> +     if (ret != 0)
>> +             return ret;
>> +     ret = platform_driver_register(&serial_omap_driver);
>> +     if (ret != 0)
>> +             uart_unregister_driver(&serial_omap_reg);
>> +     return ret;
>> +}
>> +
>> +void __exit serial_omap_exit(void)
>> +{
>> +     platform_driver_unregister(&serial_omap_driver);
>> +     uart_unregister_driver(&serial_omap_reg);
>> +}
>> +
>> +module_init(serial_omap_init);
>> +module_exit(serial_omap_exit);
>> +
>> +MODULE_DESCRIPTION("OMAP High Speed UART driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Texas Instruments Inc");
>> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>> index 8c3dd36..e02b945 100644
>> --- a/include/linux/serial_core.h
>> +++ b/include/linux/serial_core.h
>> @@ -182,6 +182,9 @@
>>  /* Aeroflex Gaisler GRLIB APBUART */
>>  #define PORT_APBUART    90
>>
>> +/* TI OMAP-UART */
>> +#define PORT_OMAP    91
>> +
>>  #ifdef __KERNEL__
>>
>>  #include <linux/compiler.h>
>> --
>> 1.6.3.3
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
---
Regards,
Govindraj.R

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html