Hi Olof,
>On Thu, Jan 28, 2010 at 8:52 AM, Olof Johansson <olof@xxxxxxxxx> 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@xxxxxx>
>> 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@xxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Govindraj R <govindraj.raja@xxxxxx>
>> ---
>> 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@xxxxxx>
>> + * Thara Gopinath <thara@xxxxxx>
>> + *
>> + * 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@xxxxxx>
>> + * Thara Gopinath <thara@xxxxxx>
>> + *
>> + * 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@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
