Re: [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

Re: [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Masakazu Mokuno]

Hi Feng,

My comments inlined.

On Thu, 4 Mar 2010 15:25:24 +0800
Feng Tang <feng.tang@intel.com> wrote:

> This is the driver for Max3110 SPI-UART device, which connect
> to host with SPI interface. It supports baud rates from 300 to
> 230400, and supports both polling and IRQ mode, as well as
> providing a console for system use
> 
> Its datasheet could be found here:
> http://datasheets.maxim-ic.com/en/ds/MAX3110E-MAX3111E.pdf
> 
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: David Brownell <david-b@pacbell.net>
> Cc: Greg KH <greg@kroah.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Acked-by: Alan Cox <alan@linux.intel.com>
> ---
>  drivers/serial/Kconfig      |    8 +
>  drivers/serial/Makefile     |    1 +
>  drivers/serial/max3110.c    |  892 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/serial/max3110.h    |   61 +++
>  include/linux/serial_core.h |    3 +
>  5 files changed, 965 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/serial/max3110.c
>  create mode 100644 drivers/serial/max3110.h
> 
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 9ff47db..94aa282 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -688,6 +688,14 @@ config SERIAL_SA1100_CONSOLE
>  	  your boot loader (lilo or loadlin) about how to pass options to the
>  	  kernel at boot time.)
>  
> +config SERIAL_MAX3110
> +	tristate "SPI UART driver for Max3110"
> +	depends on SPI_MASTER
> +	select SERIAL_CORE
> +	select SERIAL_CORE_CONSOLE
> +	help
> +	  This is the UART protocol driver for MAX3110 device
> +
>  config SERIAL_BFIN
>  	tristate "Blackfin serial port support"
>  	depends on BLACKFIN
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index 5548fe7..b93d8a0 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_SERIAL_S3C24A0) += s3c24a0.o
>  obj-$(CONFIG_SERIAL_S3C6400) += s3c6400.o
>  obj-$(CONFIG_SERIAL_S5PC100) += s3c6400.o
>  obj-$(CONFIG_SERIAL_MAX3100) += max3100.o
> +obj-$(CONFIG_SERIAL_MAX3110) += max3110.o
>  obj-$(CONFIG_SERIAL_IP22_ZILOG) += ip22zilog.o
>  obj-$(CONFIG_SERIAL_MUX) += mux.o
>  obj-$(CONFIG_SERIAL_68328) += 68328serial.o
> diff --git a/drivers/serial/max3110.c b/drivers/serial/max3110.c
> new file mode 100644
> index 0000000..9b39914
> --- /dev/null
> +++ b/drivers/serial/max3110.c
> @@ -0,0 +1,892 @@
> +/*
> + * max3110.c - spi uart protocol driver for Maxim 3110
> + *
> + * Copyright (c) 2009, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +/*
> + * Note:
> + * 1. From Max3110 spec, the Rx FIFO has 8 words, while the Tx FIFO only has
> + *    1 word. If SPI master controller doesn't support sclk frequency change,
> + *    then the char need be sent out one by one with some delay
> + *
> + * 2. Currently only RX availabe interrrupt is used
> + */
> +
> +#include <linux/module.h>
> +#include <linux/ioport.h>
> +#include <linux/init.h>
> +#include <linux/console.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial_reg.h>
> +#include <linux/kthread.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +
> +#include "max3110.h"
> +
> +#define PR_FMT	"max3110: "
> +
> +struct uart_max3110 {
> +	struct uart_port port;
> +	struct spi_device *spi;
> +	char *name;
> +
> +	wait_queue_head_t wq;
> +	struct task_struct *main_thread;
> +	struct task_struct *read_thread;
> +	spinlock_t lock;

checkpatch.pl complained:

CHECK: spinlock_t definition without comment
#119: FILE: drivers/serial/max3110.c:53:
+	spinlock_t lock;

> +
> +	u32 baud;
> +	u16 cur_conf;
> +	u8 clock;
> +	u8 parity, word_7bits;
> +	u16 irq;
> +
> +	/* bit map for UART status */
> +	unsigned long flags;
> +#define M3110_CON_TX_NEED	0
> +#define M3110_UART_TX_NEED	1
> +#define M3110_IRQ_PENDING	2
> +	unsigned long mthread_up;
> +
> +	/* console buffer */
> +	struct circ_buf con_xmit;
> +};
> +
> +static struct uart_max3110 *pmax;
> +
> +static int use_irq = 1;
> +module_param(use_irq, int, 0444);
> +MODULE_PARM_DESC(use_irq, "Whether using Max3110's IRQ capability");
> +
> +static void receive_chars(struct uart_max3110 *max,
> +				unsigned char *str, int len);
> +static int max3110_read_multi(struct uart_max3110 *max, u8 *buf);
> +static void max3110_con_receive(struct uart_max3110 *max);
> +
> +static int max3110_write_then_read(struct uart_max3110 *max,
> +		const void *txbuf, void *rxbuf, unsigned len, int always_fast)
> +{
> +	struct spi_device *spi = max->spi;
> +	struct spi_message	message;
> +	struct spi_transfer	x;
> +	int ret;
> +
> +	spi_message_init(&message);
> +	memset(&x, 0, sizeof x);
> +	x.len = len;
> +	x.tx_buf = txbuf;
> +	x.rx_buf = rxbuf;
> +	spi_message_add_tail(&x, &message);
> +
> +	if (always_fast)
> +		x.speed_hz = spi->max_speed_hz;
> +	else if (max->baud)
> +		x.speed_hz = max->baud;
> +
> +	/* Do the i/o */
> +	ret = spi_sync(spi, &message);
> +	return ret;
> +}
> +
> +/* Write a 16b word to the device */
> +static int max3110_out(struct uart_max3110 *max, const u16 out)
> +{
> +	void *buf;
> +	u16 *obuf, *ibuf;
> +	u8  ch;
> +	int ret;
> +
> +	buf = kmalloc(8, GFP_KERNEL | GFP_DMA);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	obuf = buf;
> +	ibuf = buf + 4;
> +	*obuf = out;
> +	ret = max3110_write_then_read(max, obuf, ibuf, 2, 1);
> +	if (ret) {
> +		pr_warning(PR_FMT "%s(): get err msg %d when sending 0x%x\n",
> +				__func__, ret, out);
> +		goto exit;
> +	}
> +
> +	/* If some valid data is read back */
> +	if (*ibuf & MAX3110_READ_DATA_AVAILABLE) {
> +		ch = *ibuf & 0xff;
> +		receive_chars(max, &ch, 1);
> +	}
> +
> +exit:
> +	kfree(buf);
> +	return ret;
> +}
> +
> +/*
> + * This is usually used to read data from SPIC RX FIFO, which doesn't
> + * need any delay like flushing character out.
> + *
> + * Return how many valide bytes are read back
> + */
> +static int max3110_read_multi(struct uart_max3110 *max, u8 *rxbuf)
> +{
> +	void *buf;
> +	u16 *obuf, *ibuf;
> +	u8 *pbuf, valid_str[M3110_RX_FIFO_DEPTH];
> +	int i, j, blen;
> +
> +	blen = M3110_RX_FIFO_DEPTH * sizeof(u16);
> +	buf = kmalloc(blen * 2, GFP_KERNEL | GFP_DMA);

kzalloc()

> +	if (!buf) {
> +		pr_warning(PR_FMT "%s(): fail to alloc dma buffer\n", __func__);
> +		return 0;
> +	}
> +
> +	/* tx/rx always have the same length */
> +	memset(buf, 0, blen * 2);

Then no need to do this.

> +	obuf = buf;
> +	ibuf = buf + blen;
> +
> +	if (max3110_write_then_read(max, obuf, ibuf, blen, 1)) {
> +		kfree(buf);
> +		return 0;
> +	}
> +
> +	/* If caller doesn't provide a buffer, then handle received char */
> +	pbuf = rxbuf ? rxbuf : valid_str;
> +
> +	for (i = 0, j = 0; i < M3110_RX_FIFO_DEPTH; i++) {
> +		if (ibuf[i] & MAX3110_READ_DATA_AVAILABLE)
> +			pbuf[j++] = ibuf[i] & 0xff;

		else
			break;
			
Can be added in order to optimize a bit :)
There are other similar places where search valid received chars.

> +	}
> +
> +	if (j && (pbuf == valid_str))
> +		receive_chars(max, valid_str, j);
> +
> +	kfree(buf);
> +	return j;
> +}
> +
> +static void serial_m3110_con_putchar(struct uart_port *port, int ch)
> +{
> +	struct uart_max3110 *max =
> +		container_of(port, struct uart_max3110, port);
> +	struct circ_buf *xmit = &max->con_xmit;
> +
> +	if (uart_circ_chars_free(xmit)) {
> +		xmit->buf[xmit->head] = (char)ch;
> +		xmit->head = (xmit->head + 1) & (PAGE_SIZE - 1);
> +	}
> +}
> +
> +/*
> + * Print a string to the serial port trying not to disturb
> + * any possible real use of the port...
> + *
> + *	The console_lock must be held when we get here.
> + */
> +static void serial_m3110_con_write(struct console *co,
> +				const char *s, unsigned int count)
> +{
> +	if (!pmax)
> +		return;
> +
> +	uart_console_write(&pmax->port, s, count, serial_m3110_con_putchar);
> +
> +	set_bit(M3110_CON_TX_NEED, &pmax->flags);
> +	if (!test_bit(0, &pmax->mthread_up))
> +		wake_up_process(pmax->main_thread);
> +}
> +
> +static int __init
> +serial_m3110_con_setup(struct console *co, char *options)
> +{
> +	struct uart_max3110 *max = pmax;
> +	int baud = 115200;
> +	int bits = 8;
> +	int parity = 'n';
> +	int flow = 'n';
> +
> +	pr_info(PR_FMT "setting up console\n");
> +
> +	if (!max) {
> +		pr_err(PR_FMT "pmax is NULL, return");
> +		return -ENODEV;
> +	}
> +
> +	if (options)
> +		uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> +	return uart_set_options(&max->port, co, baud, parity, bits, flow);
> +}
> +
> +static struct tty_driver *serial_m3110_con_device(struct console *co,
> +							int *index)
> +{
> +	struct uart_driver *p = co->data;
> +	*index = co->index;
> +	return p->tty_driver;
> +}
> +
> +static struct uart_driver serial_m3110_reg;
> +static struct console serial_m3110_console = {
> +	.name		= "ttyS",
> +	.write		= serial_m3110_con_write,
> +	.device		= serial_m3110_con_device,
> +	.setup		= serial_m3110_con_setup,
> +	.flags		= CON_PRINTBUFFER,
> +	.index		= -1,
> +	.data		= &serial_m3110_reg,
> +};
> +
> +
> +static unsigned int serial_m3110_tx_empty(struct uart_port *port)
> +{
> +	return 1;
> +}
> +
> +static void serial_m3110_stop_tx(struct uart_port *port)
> +{
> +	return;
> +}
> +
> +static void serial_m3110_stop_rx(struct uart_port *port)
> +{
> +	return;
> +}
> +
> +#define WORDS_PER_XFER	128
> +static void send_circ_buf(struct uart_max3110 *max,
> +				struct circ_buf *xmit)
> +{
> +	void *buf;
> +	u16 *obuf, *ibuf;
> +	u8 valid_str[WORDS_PER_XFER];
> +	int i, j, len, blen, dma_size, left, ret = 0;
> +
> +
> +	dma_size = WORDS_PER_XFER * sizeof(u16) * 2;
> +	buf = kmalloc(dma_size, GFP_KERNEL | GFP_DMA);
> +	if (!buf)
> +		return;
> +	obuf = buf;
> +	ibuf = buf + dma_size/2;
> +
> +	while (!uart_circ_empty(xmit)) {
> +		left = uart_circ_chars_pending(xmit);
> +		while (left) {
> +			len = min(left, WORDS_PER_XFER);
> +			blen = len * sizeof(u16);
> +			memset(obuf, 0, blen);

I don't think repeated zeroing TX buffer is needed. Once cleared, it
keeps zeroed in this function.

> +			memset(ibuf, 0, blen);
> +
> +			for (i = 0; i < len; i++) {
> +				obuf[i] = (u8)xmit->buf[xmit->tail] | WD_TAG;
> +				xmit->tail = (xmit->tail + 1) &
> +						(UART_XMIT_SIZE - 1);
> +			}
> +
> +			/* Fail to send msg to console is not very critical */
> +			ret = max3110_write_then_read(max, obuf, ibuf, blen, 0);
> +			if (ret)
> +				pr_warning(PR_FMT "%s(): get err msg %d\n",
> +						__func__, ret);
> +
> +			for (i = 0, j = 0; i < len; i++) {
> +				if (ibuf[i] & MAX3110_READ_DATA_AVAILABLE)
> +					valid_str[j++] = ibuf[i] & 0xff;
> +			}
> +
> +			if (j)
> +				receive_chars(max, valid_str, j);
> +
> +			max->port.icount.tx += len;
> +			left -= len;
> +		}
> +	}
> +
> +	kfree(buf);
> +}
> +
> +static void transmit_char(struct uart_max3110 *max)
> +{
> +	struct uart_port *port = &max->port;
> +	struct circ_buf *xmit = &port->state->xmit;
> +
> +	if (uart_circ_empty(xmit) || uart_tx_stopped(port))
> +		return;
> +
> +	send_circ_buf(max, xmit);
> +
> +	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> +		uart_write_wakeup(port);
> +}
> +
> +/*
> + * This will be called by uart_write() and tty_write, can't
> + * go to sleep
> + */
> +static void serial_m3110_start_tx(struct uart_port *port)
> +{
> +	struct uart_max3110 *max =
> +		container_of(port, struct uart_max3110, port);
> +
> +	set_bit(M3110_UART_TX_NEED, &max->flags);
> +	if (!test_bit(0, &max->mthread_up))
> +		wake_up_process(max->main_thread);
> +}
> +
> +static void receive_chars(struct uart_max3110 *max, unsigned char *str, int len)
> +{
> +	struct uart_port *port = &max->port;
> +	struct tty_struct *tty;
> +	int usable;
> +
> +	/* If uart is not opened, just return */
> +	if (!port->state)
> +		return;
> +
> +	tty = port->state->port.tty;
> +	if (!tty)
> +		return;
> +
> +	while (len) {
> +		usable = tty_buffer_request_room(tty, len);
> +		if (usable) {
> +			tty_insert_flip_string(tty, str, usable);
> +			str += usable;
> +			port->icount.rx += usable;
> +		}
> +		len -= usable;
> +	}
> +	tty_flip_buffer_push(tty);
> +}
> +
> +/*
> + * This routine will be used in read_thread or RX IRQ handling,
> + * it will first do one round buffer read(8 words), if there is some
> + * valid RX data, will try to read 5 more rounds till all data
> + * is read out.
> + *
> + * Use stack space as data buffer to save some system load, and chose
> + * 504 Btyes as a threadhold to do a bulk push to upper tty layer when
> + * receiving bulk data, a much bigger buffer may cause stack overflow
> + */
> +static void max3110_con_receive(struct uart_max3110 *max)
> +{
> +	int loop = 1, num, total = 0;
> +	u8 recv_buf[512], *pbuf;
> +
> +	pbuf = recv_buf;
> +	do {
> +		num = max3110_read_multi(max, pbuf);
> +
> +		if (num) {
> +			loop = 5;
> +			pbuf += num;
> +			total += num;
> +
> +			if (total >= 504) {
> +				receive_chars(max, recv_buf, total);
> +				pbuf = recv_buf;
> +				total = 0;
> +			}
> +		}
> +	} while (--loop);
> +
> +	if (total)
> +		receive_chars(max, recv_buf, total);
> +}
> +
> +static int max3110_main_thread(void *_max)
> +{
> +	struct uart_max3110 *max = _max;
> +	wait_queue_head_t *wq = &max->wq;
> +	int ret = 0;
> +	struct circ_buf *xmit = &max->con_xmit;
> +
> +	init_waitqueue_head(wq);
> +	pr_info(PR_FMT "start main thread\n");
> +
> +	do {
> +		wait_event_interruptible(*wq,
> +				max->flags || kthread_should_stop());
> +		test_and_set_bit(0, &max->mthread_up);

The result of testing ignored. Why testing?

> +
> +		if (use_irq && test_bit(M3110_IRQ_PENDING, &max->flags)) {
> +			max3110_con_receive(max);
> +			clear_bit(M3110_IRQ_PENDING, &max->flags);
> +		}

test_and_clear_bit()?

> +
> +		/* First handle console output */
> +		if (test_bit(M3110_CON_TX_NEED, &max->flags)) {
> +			send_circ_buf(max, xmit);
> +			clear_bit(M3110_CON_TX_NEED, &max->flags);
> +		}

Ditto

> +
> +		/* Handle uart output */
> +		if (test_bit(M3110_UART_TX_NEED, &max->flags)) {
> +			transmit_char(max);
> +			clear_bit(M3110_UART_TX_NEED, &max->flags);
> +		}

Ditto

> +		test_and_clear_bit(0, &max->mthread_up);

The result ignored

> +	} while (!kthread_should_stop());
> +
> +	return ret;
> +}
> +
> +static irqreturn_t serial_m3110_irq(int irq, void *dev_id)
> +{
> +	struct uart_max3110 *max = dev_id;
> +
> +	/* max3110's irq is a falling edge, not level triggered,
> +	 * so no need to disable the irq */
> +	set_bit(M3110_IRQ_PENDING, &max->flags);
> +
> +	if (!test_bit(0, &max->mthread_up))
> +		wake_up_process(max->main_thread);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* If don't use RX IRQ, then need a thread to polling read */
> +static int max3110_read_thread(void *_max)
> +{
> +	struct uart_max3110 *max = _max;
> +
> +	pr_info(PR_FMT "start read thread\n");
> +	do {
> +		if (!test_bit(0, &max->mthread_up))
> +			max3110_con_receive(max);
> +
> +		schedule_timeout_interruptible(HZ / 20);
> +	} while (!kthread_should_stop());
> +
> +	return 0;
> +}
> +
> +static int serial_m3110_startup(struct uart_port *port)
> +{
> +	struct uart_max3110 *max =
> +		container_of(port, struct uart_max3110, port);
> +	u16 config = 0;
> +	int ret = 0;
> +
> +	if (port->line != 0) {
> +		pr_err(PR_FMT "uart port startup failed\n");
> +		return -1;
> +	}
> +
> +	/* Disable all IRQ and config it to 115200, 8n1 */
> +	config = WC_TAG | WC_FIFO_ENABLE
> +			| WC_1_STOPBITS
> +			| WC_8BIT_WORD
> +			| WC_BAUD_DR2;
> +
> +	/* As we use thread to handle tx/rx, need set low latency */
> +	port->state->port.tty->low_latency = 1;
> +
> +	if (use_irq) {
> +		ret = request_irq(max->irq, serial_m3110_irq,
> +					IRQ_TYPE_EDGE_FALLING, "max3110", max);

According to the manufacturer's datasheet, it looks like MAX3110'irq is
level interrupt. Refer Figure 6 of the datasheet.

> +		if (ret)
> +			return ret;
> +
> +		/* Enable RX IRQ only */
> +		config |= WC_RXA_IRQ_ENABLE;
> +	} else {
> +		/* If IRQ is disabled, start a read thread for input data */
> +		max->read_thread =
> +			kthread_run(max3110_read_thread, max, "max3110_read");
> +		if (IS_ERR(max->read_thread)) {
> +			ret = PTR_ERR(max->read_thread);
> +			max->read_thread = 0;
> +			pr_err(PR_FMT "Can't create read thread!");
> +			return ret;
> +		}
> +	}
> +
> +	ret = max3110_out(max, config);
> +	if (ret) {
> +		if (use_irq)
> +			free_irq(max->irq, max);
> +		else
> +			kthread_stop(max->read_thread);
> +
> +		return ret;
> +	}
> +
> +	max->cur_conf = config;
> +	return 0;
> +}
> +
> +static void serial_m3110_shutdown(struct uart_port *port)
> +{
> +	struct uart_max3110 *max =
> +		container_of(port, struct uart_max3110, port);
> +	u16 config;
> +
> +	if (max->read_thread) {
> +		kthread_stop(max->read_thread);
> +		max->read_thread = NULL;
> +	}
> +
> +	if (use_irq)
> +		free_irq(max->irq, max);
> +
> +	/* Disable interrupts from this port */
> +	config = WC_TAG | WC_SW_SHDI;
> +	max3110_out(max, config);
> +}
> +
> +static void serial_m3110_release_port(struct uart_port *port)
> +{
> +}
> +
> +static int serial_m3110_request_port(struct uart_port *port)
> +{
> +	return 0;
> +}
> +
> +static void serial_m3110_config_port(struct uart_port *port, int flags)
> +{
> +	port->type = PORT_MAX3110;
> +}
> +
> +static int
> +serial_m3110_verify_port(struct uart_port *port, struct serial_struct *ser)
> +{
> +	/* We don't want the core code to modify any port params */
> +	return -EINVAL;
> +}
> +
> +
> +static const char *serial_m3110_type(struct uart_port *port)
> +{
> +	struct uart_max3110 *max =
> +		container_of(port, struct uart_max3110, port);
> +	return max->name;
> +}
> +
> +static void
> +serial_m3110_set_termios(struct uart_port *port, struct ktermios *termios,
> +		       struct ktermios *old)
> +{
> +	struct uart_max3110 *max =
> +		container_of(port, struct uart_max3110, port);
> +	unsigned char cval;
> +	unsigned int baud, parity = 0;
> +	int clk_div = -1;
> +	u16 new_conf = max->cur_conf;
> +
> +	switch (termios->c_cflag & CSIZE) {
> +	case CS7:
> +		cval = UART_LCR_WLEN7;
> +		new_conf |= WC_7BIT_WORD;
> +		break;
> +	default:
> +		/* We only support CS7 & CS8 */
> +		termios->c_cflag &= ~CSIZE;
> +		termios->c_cflag |= CS8;
> +	case CS8:
> +		cval = UART_LCR_WLEN8;
> +		new_conf |= WC_8BIT_WORD;
> +		break;
> +	}
> +
> +	baud = uart_get_baud_rate(port, termios, old, 0, 230400);
> +
> +	/* First calc the div for 1.8MHZ clock case */
> +	switch (baud) {
> +	case 300:
> +		clk_div = WC_BAUD_DR384;
> +		break;
> +	case 600:
> +		clk_div = WC_BAUD_DR192;
> +		break;
> +	case 1200:
> +		clk_div = WC_BAUD_DR96;
> +		break;
> +	case 2400:
> +		clk_div = WC_BAUD_DR48;
> +		break;
> +	case 4800:
> +		clk_div = WC_BAUD_DR24;
> +		break;
> +	case 9600:
> +		clk_div = WC_BAUD_DR12;
> +		break;
> +	case 19200:
> +		clk_div = WC_BAUD_DR6;
> +		break;
> +	case 38400:
> +		clk_div = WC_BAUD_DR3;
> +		break;
> +	case 57600:
> +		clk_div = WC_BAUD_DR2;
> +		break;
> +	case 115200:
> +		clk_div = WC_BAUD_DR1;
> +		break;
> +	case 230400:
> +		if (max->clock & MAX3110_HIGH_CLK)
> +			break;
> +	default:
> +		/* Pick the previous baud rate */
> +		baud = max->baud;
> +		clk_div = max->cur_conf & WC_BAUD_DIV_MASK;
> +		tty_termios_encode_baud_rate(termios, baud, baud);
> +	}
> +
> +	if (max->clock & MAX3110_HIGH_CLK) {
> +		clk_div += 1;
> +		/* High clk version max3110 doesn't support B300 */
> +		if (baud == 300)
> +			baud = 600;

As (WC_BAUD_DR384 + 1) is 0x10, need to reset as 0xf like:

		if (baud == 300) {
			baud = 600;
			clk_div = WC_BAUD_DR384;
		}

> +		if (baud == 230400)
> +			clk_div = WC_BAUD_DR1;

This if statement can be omitted as WC_BAUD_DR1 is 0 and clk_div becomes
(-1 + 1).

> +		tty_termios_encode_baud_rate(termios, baud, baud);
> +	}
> +
> +	new_conf = (new_conf & ~WC_BAUD_DIV_MASK) | clk_div;
> +
> +	if (unlikely(termios->c_cflag & CMSPAR))
> +		termios->c_cflag &= ~CMSPAR;
> +
> +	if (termios->c_cflag & CSTOPB)
> +		new_conf |= WC_2_STOPBITS;
> +	else
> +		new_conf &= ~WC_2_STOPBITS;
> +
> +	if (termios->c_cflag & PARENB) {
> +		new_conf |= WC_PARITY_ENABLE;
> +		parity |= UART_LCR_PARITY;
> +	} else
> +		new_conf &= ~WC_PARITY_ENABLE;
> +
> +	if (!(termios->c_cflag & PARODD))
> +		parity |= UART_LCR_EPAR;
> +	max->parity = parity;
> +
> +	uart_update_timeout(port, termios->c_cflag, baud);
> +
> +	new_conf |= WC_TAG;
> +	if (new_conf != max->cur_conf) {
> +		if (!max3110_out(max, new_conf)) {
> +			max->cur_conf = new_conf;
> +			max->baud = baud;
> +		}
> +	}
> +}
> +
> +/* Don't handle hw handshaking */
> +static unsigned int serial_m3110_get_mctrl(struct uart_port *port)
> +{
> +	return TIOCM_DSR | TIOCM_CAR | TIOCM_DSR;
> +}
> +
> +static void serial_m3110_set_mctrl(struct uart_port *port, unsigned int mctrl)
> +{
> +}
> +
> +static void serial_m3110_break_ctl(struct uart_port *port, int break_state)
> +{
> +}
> +
> +static void serial_m3110_pm(struct uart_port *port, unsigned int state,
> +			unsigned int oldstate)
> +{
> +}
> +
> +static void serial_m3110_enable_ms(struct uart_port *port)
> +{
> +}
> +
> +struct uart_ops serial_m3110_ops = {
> +	.tx_empty	= serial_m3110_tx_empty,
> +	.set_mctrl	= serial_m3110_set_mctrl,
> +	.get_mctrl	= serial_m3110_get_mctrl,
> +	.stop_tx	= serial_m3110_stop_tx,
> +	.start_tx	= serial_m3110_start_tx,
> +	.stop_rx	= serial_m3110_stop_rx,
> +	.enable_ms	= serial_m3110_enable_ms,
> +	.break_ctl	= serial_m3110_break_ctl,
> +	.startup	= serial_m3110_startup,
> +	.shutdown	= serial_m3110_shutdown,
> +	.set_termios	= serial_m3110_set_termios,
> +	.pm		= serial_m3110_pm,
> +	.type		= serial_m3110_type,
> +	.release_port	= serial_m3110_release_port,
> +	.request_port	= serial_m3110_request_port,
> +	.config_port	= serial_m3110_config_port,
> +	.verify_port	= serial_m3110_verify_port,
> +};
> +
> +static struct uart_driver serial_m3110_reg = {
> +	.owner		= THIS_MODULE,
> +	.driver_name	= "Maxim 3110",
> +	.dev_name	= "ttyS",
> +	.major		= TTY_MAJOR,
> +	.minor		= 64,
> +	.nr		= 1,
> +	.cons		= &serial_m3110_console,
> +};
> +
> +#ifdef CONFIG_PM
> +static int serial_m3110_suspend(struct spi_device *spi, pm_message_t state)
> +{
> +	disable_irq(pmax->irq);
> +	uart_suspend_port(&serial_m3110_reg, &pmax->port);
> +	max3110_out(pmax, pmax->cur_conf | WC_SW_SHDI);
> +	return 0;
> +}
> +
> +static int serial_m3110_resume(struct spi_device *spi)
> +{
> +	max3110_out(pmax, pmax->cur_conf);
> +	uart_resume_port(&serial_m3110_reg, &pmax->port);
> +	enable_irq(pmax->irq);
> +	return 0;
> +}
> +#else
> +#define serial_m3110_suspend	NULL
> +#define serial_m3110_resume	NULL
> +#endif
> +
> +static int __devinit serial_m3110_probe(struct spi_device *spi)
> +{
> +	struct uart_max3110 *max;
> +	int ret;
> +	void *buffer;
> +
> +	max = kzalloc(sizeof(*max), GFP_KERNEL);
> +	if (!max)
> +		return -ENOMEM;
> +
> +	spi->bits_per_word = 16;
> +	spi_setup(spi);
> +
> +	max->clock = MAX3110_HIGH_CLK;

Would it be better if this was configurable or a module parameter for
implementations which used 1.8MHz clock?

> +	max->port.type = PORT_MAX3110;
> +	max->port.fifosize = 2;		/* Only have 16b buffer */

I guess MAX3110 has 8 chars RX FIFO and no TX FIFO.  If so, value 2 is
OK here?

> +	max->port.ops = &serial_m3110_ops;
> +	max->port.line = 0;
> +	max->port.dev = &spi->dev;
> +	max->port.uartclk = 115200;
> +
> +	max->spi = spi;
> +	max->name = spi->modalias;	/* Use spi name as the name */
> +	max->irq = (u16)spi->irq;
> +	spin_lock_init(&max->lock);
> +
> +	max->word_7bits = 0;
> +	max->parity = 0;
> +	max->baud = 0;
> +
> +	max->cur_conf = 0;
> +	max->flags = 0;
> +
> +	buffer = (void *)__get_free_page(GFP_KERNEL);
> +	if (!buffer) {
> +		ret = -ENOMEM;
> +		goto err_get_page;
> +	}
> +	max->con_xmit.buf = buffer;
> +	max->con_xmit.head = max->con_xmit.tail = 0;

checkpatch.pl complained:

WARNING: multiple assignments should be avoided
#877: FILE: drivers/serial/max3110.c:811:
+	max->con_xmit.head = max->con_xmit.tail = 0;


> +
> +	max->main_thread = kthread_run(max3110_main_thread,
> +					max, "max3110_main");
> +	if (IS_ERR(max->main_thread)) {
> +		ret = PTR_ERR(max->main_thread);
> +		goto err_kthread;
> +	}
> +
> +	pmax = max;

If this driver supports only one instance of devices, how about
declaring a global struct uart_m3100 instead of kmallc()?

> +	/* Give membase a psudo value to pass serial_core's check */
> +	max->port.membase = buffer;
> +	uart_add_one_port(&serial_m3110_reg, &max->port);
> +
> +	return 0;
> +
> +err_kthread:
> +	free_page((unsigned long)buffer);
> +err_get_page:
> +	pmax = NULL;
> +	kfree(max);
> +	return ret;
> +}
> +
> +static int __devexit max3110_remove(struct spi_device *dev)
> +{
> +	struct uart_max3110 *max = pmax;

Or use dev_set_drvdata(&spi->dev, max) in probe() and retrieve it with
dev_get_drvdata(&spi->dev) when it is needed?

> +
> +	if (!pmax)
> +		return 0;
> +
> +	pmax = NULL;
> +	uart_remove_one_port(&serial_m3110_reg, &max->port);
> +
> +	free_page((unsigned long)max->con_xmit.buf);
> +
> +	if (max->main_thread)
> +		kthread_stop(max->main_thread);
> +
> +	kfree(max);
> +	return 0;
> +}
> +
> +static struct spi_driver uart_max3110_driver = {
> +	.driver = {
> +			.name	= "spi_max3110",
> +			.bus	= &spi_bus_type,
> +			.owner	= THIS_MODULE,
> +	},
> +	.probe		= serial_m3110_probe,
> +	.remove		= __devexit_p(max3110_remove),
> +	.suspend	= serial_m3110_suspend,
> +	.resume		= serial_m3110_resume,
> +};
> +
> +static int __init serial_m3110_init(void)
> +{
> +	int ret = 0;
> +
> +	ret = uart_register_driver(&serial_m3110_reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_register_driver(&uart_max3110_driver);
> +	if (ret)
> +		uart_unregister_driver(&serial_m3110_reg);
> +
> +	return ret;
> +}
> +
> +static void __exit serial_m3110_exit(void)
> +{
> +	spi_unregister_driver(&uart_max3110_driver);
> +	uart_unregister_driver(&serial_m3110_reg);
> +}
> +
> +module_init(serial_m3110_init);
> +module_exit(serial_m3110_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("max3110-uart");
> +MODULE_AUTHOR("Feng Tang <feng.tang@intel.com>");
> diff --git a/drivers/serial/max3110.h b/drivers/serial/max3110.h
> new file mode 100644
> index 0000000..4d58641
> --- /dev/null
> +++ b/drivers/serial/max3110.h
> @@ -0,0 +1,61 @@
> +#ifndef _MAX3110_HEAD_FILE_
> +#define _MAX3110_HEAD_FILE_
> +
> +#define MAX3110_HIGH_CLK	0x1	/* 3.6864 MHZ */
> +#define MAX3110_LOW_CLK		0x0	/* 1.8432 MHZ */
> +
> +/* Status bits for all 4 MAX3110 operate modes */
> +#define MAX3110_READ_DATA_AVAILABLE	(1 << 15)
> +#define MAX3110_WRITE_BUF_EMPTY		(1 << 14)
> +
> +#define WC_TAG			(3 << 14)
> +#define RC_TAG			(1 << 14)
> +#define WD_TAG			(2 << 14)
> +#define RD_TAG			(0 << 14)
> +
> +/* Bits def for write configuration */
> +#define WC_FIFO_ENABLE_MASK	(1 << 13)
> +#define WC_FIFO_ENABLE		(0 << 13)
> +
> +#define WC_SW_SHDI		(1 << 12)
> +
> +#define WC_IRQ_MASK		(0xF << 8)
> +#define WC_TXE_IRQ_ENABLE	(1 << 11)	/* TX empty irq */
> +#define WC_RXA_IRQ_ENABLE	(1 << 10)	/* RX availabe irq */
> +#define WC_PAR_HIGH_IRQ_ENABLE	(1 << 9)
> +#define WC_REC_ACT_IRQ_ENABLE	(1 << 8)
> +
> +#define WC_IRDA_ENABLE		(1 << 7)
> +
> +#define WC_STOPBITS_MASK	(1 << 6)
> +#define WC_2_STOPBITS		(1 << 6)
> +#define WC_1_STOPBITS		(0 << 6)
> +
> +#define WC_PARITY_ENABLE_MASK	(1 << 5)
> +#define WC_PARITY_ENABLE	(1 << 5)
> +
> +#define WC_WORDLEN_MASK		(1 << 4)
> +#define WC_7BIT_WORD		(1 << 4)
> +#define WC_8BIT_WORD		(0 << 4)
> +
> +#define WC_BAUD_DIV_MASK	(0xF)
> +#define WC_BAUD_DR1		(0x0)
> +#define WC_BAUD_DR2		(0x1)
> +#define WC_BAUD_DR4		(0x2)
> +#define WC_BAUD_DR8		(0x3)
> +#define WC_BAUD_DR16		(0x4)
> +#define WC_BAUD_DR32		(0x5)
> +#define WC_BAUD_DR64		(0x6)
> +#define WC_BAUD_DR128		(0x7)
> +#define WC_BAUD_DR3		(0x8)
> +#define WC_BAUD_DR6		(0x9)
> +#define WC_BAUD_DR12		(0xA)
> +#define WC_BAUD_DR24		(0xB)
> +#define WC_BAUD_DR48		(0xC)
> +#define WC_BAUD_DR96		(0xD)
> +#define WC_BAUD_DR192		(0xE)
> +#define WC_BAUD_DR384		(0xF)
> +
> +/* Maxim 3110 has 8 words RX FIFO and 1 word TX FIFO */
> +#define M3110_RX_FIFO_DEPTH	8
> +#endif
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 8c3dd36..119ba73 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
>  
> +/* Maxim M3110 */
> +#define PORT_MAX3110	91
> +
>  #ifdef __KERNEL__
>  
>  #include <linux/compiler.h>
> -- 
> 1.6.3.3
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Masakazu Mokuno

Re: [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Feng Tang]

Hi Mokuno,

Thanks for the thorough review and good comments!
Some of your comments are answered inline, others will be addressed in future
 patch submission.

I would give v6 of this patch more time to be reviewed by community, so that
more comments can come and I can handle them altogether.

Thanks,
Feng

On Fri, 5 Mar 2010 02:46:07 +0800
Masakazu Mokuno <mokuno@sm.sony.co.jp> wrote:

> Hi Feng,
> 
> My comments inlined.
> 
> On Thu, 4 Mar 2010 15:25:24 +0800
> Feng Tang <feng.tang@intel.com> wrote:
> 

> > +     wait_queue_head_t wq;
> > +     struct task_struct *main_thread;
> > +     struct task_struct *read_thread;
> > +     spinlock_t lock;
> 
> checkpatch.pl complained:
> 
> CHECK: spinlock_t definition without comment
> #119: FILE: drivers/serial/max3110.c:53:
> +       spinlock_t lock;

A little strange, I'm using checkpatch.pl in kernel source, which doesn't
generate any warning. Which version are you using?



> > +
> > +     /* If caller doesn't provide a buffer, then handle received
> > char */
> > +     pbuf = rxbuf ? rxbuf : valid_str;
> > +
> > +     for (i = 0, j = 0; i < M3110_RX_FIFO_DEPTH; i++) {
> > +             if (ibuf[i] & MAX3110_READ_DATA_AVAILABLE)
> > +                     pbuf[j++] = ibuf[i] & 0xff;
> 
>                 else
>                         break;
> 
> Can be added in order to optimize a bit :)
> There are other similar places where search valid received chars.

Reading Max3110 buffer is asynchronous operation, like for the reader thread,
spi controller will write 8 words of 0 to max3110, and read 8 words back, the
first word doesn't have a valid data doesn't mean the following don't, and we
need to check them all


> > +static int max3110_main_thread(void *_max)
> > +{
> > +     struct uart_max3110 *max = _max;
> > +     wait_queue_head_t *wq = &max->wq;
> > +     int ret = 0;
> > +     struct circ_buf *xmit = &max->con_xmit;
> > +
> > +     init_waitqueue_head(wq);
> > +     pr_info(PR_FMT "start main thread\n");
> > +
> > +     do {
> > +             wait_event_interruptible(*wq,
> > +                             max->flags || kthread_should_stop());
> > +             test_and_set_bit(0, &max->mthread_up);
> 
> The result of testing ignored. Why testing?

Andrew mentioned a race condition for using set/test_bit for mthread_up
flag, which I don't quiet follow due to my lack of SMP race experience,
mthread_up is not designed to make some code exclusive. And whether the main
thread can be woken up to run depends on the max->flags setting.

> 
> > +
> > +             if (use_irq && test_bit(M3110_IRQ_PENDING,
> > &max->flags)) {
> > +                     max3110_con_receive(max);
> > +                     clear_bit(M3110_IRQ_PENDING, &max->flags);
> > +             }
> 
> test_and_clear_bit()?
You mean 
	if (use_irq && test_and_clear_bit(..))
		max3110_con_receive(max);
?

Yes, it's better. I put clear_bit() after max3110_con_receive() because
I test that bit to decide whether we need wakeup the main thread again
in the ISR in old version submission.

> > +
> > +     /* As we use thread to handle tx/rx, need set low latency */
> > +     port->state->port.tty->low_latency = 1;
> > +
> > +     if (use_irq) {
> > +             ret = request_irq(max->irq, serial_m3110_irq,
> > +                                     IRQ_TYPE_EDGE_FALLING,
> > "max3110", max);
> 
> According to the manufacturer's datasheet, it looks like MAX3110'irq
> is level interrupt. Refer Figure 6 of the datasheet.

Yep, good catch here, if there is RX data in buffer, the IRQ pin will
be asserted low always. But the IRQ line will usually be connected to
system through a GPIO pin, we use falling edge for GPIO pin IRQ trigger,
when 3110's IRQ is asserted, that GPIO pin will be changed from high to low,
the ISR and following handler will try to read all the RX data out to
make 3110's IRQ go back to high, waiting for another IRQ. 

> > +             if (baud == 230400)
> > +                     clk_div = WC_BAUD_DR1;
> 
> This if statement can be omitted as WC_BAUD_DR1 is 0 and clk_div
> becomes (-1 + 1).
Nice trick, but it may confuse readers without any explanation :)


> > +     max->port.type = PORT_MAX3110;
> > +     max->port.fifosize = 2;         /* Only have 16b buffer */
> 
> I guess MAX3110 has 8 chars RX FIFO and no TX FIFO.  If so, value 2 is
> OK here?
> 
If you check the Figure 1 of the data sheet, there is still a TX buffer
there tough only one word :) or should I change it to 1? 

> 
> 
> > +
> > +     max->main_thread = kthread_run(max3110_main_thread,
> > +                                     max, "max3110_main");
> > +     if (IS_ERR(max->main_thread)) {
> > +             ret = PTR_ERR(max->main_thread);
> > +             goto err_kthread;
> > +     }
> > +
> > +     pmax = max;
> 
> If this driver supports only one instance of devices, how about
> declaring a global struct uart_m3100 instead of kmallc()?
If using global struct, the space will be allocated no matter the system
really have a Max3110, putting the allocation in probe() is flexible

Re: [spi-devel-general] [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Erwin Authried]

Am Donnerstag, den 04.03.2010, 15:25 +0800 schrieb Feng Tang:
> Hi All,
> 
> Here is a driver for Maxim 3110 SPI-UART device, please help to review.
> 
> It has been validated with Designware SPI controller (drivers/spi: dw_spi.c &
> dw_spi_pci.c). It supports polling and IRQ mode, supports batch read, and
> provides a console.
> 
> change log:
>  since v5:
>         * Make the spi data buffer DMA safe, thanks to Mssakazu Mokuno, 
>           David Brownell and Grant Likely for pointing the bug out
>  since v4:
>         * Add explanation why not using DMA
>         * Fix a bug in max3110_read_multi()
>  since v3:
>         * Remove the Designware controller related stuff
>         * Remove some initialization code which should be done in the platform
>           setup code
>         * Add a missing change for drivers/serial/Makefile
>  since v2:
>         * Address comments from Andrew Morton
>         * Use test/set_bit to avoid race condition for SMP/preempt case
>         * Fix bug related with endian order
>  since v1:
>         * Address comments from Alan Cox
>         * Use a "use_irq" module parameter to runtime check whether
>           to use IRQ mode
>         * Add the suspend/resume implementation
> 
> Thanks!
> Feng

Hi,

the only thing that I still don't like is the way of sending characters.
Basically, the spi rate is set to the actual baud rate when characters
are transmitted to the uart, and multiple characters are sent without
looking at the status of the uart.

The spi rate and the uart clock aren't synchronized, what happens if the
spi rate is slightly higher than the MAX3100's baud rate clock?
In addition, if there are other devices on the spi bus, the bus will be
occupied unnecessarily long, especially when low baudrates are used.

One other small cosmetic thing:
in serial_m3110_tx_empty(), TIOCSER_TEMT should be returned instead of
1.

-Erwin

Re: [spi-devel-general] [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Feng Tang]

On Fri, 5 Mar 2010 15:44:50 +0800
Erwin Authried <eauth@softsys.co.at> wrote:

> 
> The spi rate and the uart clock aren't synchronized, what happens if
> the spi rate is slightly higher than the MAX3100's baud rate clock?
> In addition, if there are other devices on the spi bus, the bus will
> be occupied unnecessarily long, especially when low baudrates are
> used.
Hi Erwin,

Yep, here you touched a key point of the driver.

In the early version of our driver, we used a fixed spi rate which is a
little owner than its own working rate (3.684MHz) as a spi controller can
hardly get a divided rate exactly same as 3110's clock , with that we can only
handle one word per spi transfer, and have to add a udelay function for each
transfer, and we even have to calculate the delay time for all kinds of the
UART baud rate it's working with. struct spi_transfer has a member "deay_usecs"
just for this case.

One key point for SPI devices is they can't be accessed by CPU directly and has
to go though tasklet or workqueue for each spi transfer. To improve performance,
we chose to use buffer read/write to let one transfer contain more data, and use
dynamic spi rate by setting "spi_transfer->speed_hz" for each baud rate
accordingly. spi rate is set by spi controller drivers based on the "speed_hz"
number and they should chose a lower spi rate than 3110's baud rate if can't find
a same rate. For a spi controller support multiple devices including 3110, the
delay from 3110 is inevitable no matter we use a explicit udelay or the dynamic
spi rate.

Till here, I have to say the "spi_transfer" of spi core is well designed, which
provides bits_per_word/delay_usecs/speed_hz of great flexibility for developing
spi device/controller drivers

Thanks,
Feng


> 
> One other small cosmetic thing:
> in serial_m3110_tx_empty(), TIOCSER_TEMT should be returned instead of
> 1.
> 
> -Erwin
> 
> 
> 

Re: [spi-devel-general] [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Grant Likely]

On Sun, Mar 7, 2010 at 7:11 PM, Feng Tang <feng.tang@intel.com> wrote:
> On Fri, 5 Mar 2010 15:44:50 +0800
> Erwin Authried <eauth@softsys.co.at> wrote:
>
>>
>> The spi rate and the uart clock aren't synchronized, what happens if
>> the spi rate is slightly higher than the MAX3100's baud rate clock?
>> In addition, if there are other devices on the spi bus, the bus will
>> be occupied unnecessarily long, especially when low baudrates are
>> used.
> Hi Erwin,
>
> Yep, here you touched a key point of the driver.
>
> In the early version of our driver, we used a fixed spi rate which is a
> little owner than its own working rate (3.684MHz) as a spi controller can
> hardly get a divided rate exactly same as 3110's clock , with that we can only
> handle one word per spi transfer, and have to add a udelay function for each
> transfer, and we even have to calculate the delay time for all kinds of the
> UART baud rate it's working with. struct spi_transfer has a member "deay_usecs"
> just for this case.
>
> One key point for SPI devices is they can't be accessed by CPU directly and has
> to go though tasklet or workqueue for each spi transfer. To improve performance,
> we chose to use buffer read/write to let one transfer contain more data, and use
> dynamic spi rate by setting "spi_transfer->speed_hz" for each baud rate
> accordingly. spi rate is set by spi controller drivers based on the "speed_hz"
> number and they should chose a lower spi rate than 3110's baud rate if can't find
> a same rate. For a spi controller support multiple devices including 3110, the
> delay from 3110 is inevitable no matter we use a explicit udelay or the dynamic
> spi rate.
>
> Till here, I have to say the "spi_transfer" of spi core is well designed, which
> provides bits_per_word/delay_usecs/speed_hz of great flexibility for developing
> spi device/controller drivers

You're abusing the speed_hz setting of the SPI controller.  By
clocking down the SPI bus, you're effectively using the SPI completion
interrupt as a high resolution timer to schedule your character
transmissions so that you don't overrun the max3110 which has no tx
fifo.  Using a udelay is also the wrong thing to do because it chews
up CPU cycles to achieve the same result.

The correct thing to do is to either use a high resolution timer, or
the IRQ from the MAX3110 itself to schedule your spi transfers, and
set the SPI rate to the highest frequency that the MAX3110 can handle
so that your driver neither hogs the SPI bus, nor wastes CPU cycles.
You can improve the performance of the driver by using spi_async()
instead of spi_sync() it enqueue the transfers so that you can handle
everything at IRQ context and you don't need to drop into a workqueue
to process the data.

g.

Re: [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Feng Tang]

On Mon, 8 Mar 2010 12:12:34 +0800
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:


> >
> > Yep, here you touched a key point of the driver.
> >
> > In the early version of our driver, we used a fixed spi rate which
> > is a little owner than its own working rate (3.684MHz) as a spi
> > controller can hardly get a divided rate exactly same as 3110's
> > clock , with that we can only handle one word per spi transfer, and
> > have to add a udelay function for each transfer, and we even have
> > to calculate the delay time for all kinds of the UART baud rate
> > it's working with. struct spi_transfer has a member "deay_usecs"
> > just for this case.
> >
> > One key point for SPI devices is they can't be accessed by CPU
> > directly and has to go though tasklet or workqueue for each spi
> > transfer. To improve performance, we chose to use buffer read/write
> > to let one transfer contain more data, and use dynamic spi rate by
> > setting "spi_transfer->speed_hz" for each baud rate accordingly.
> > spi rate is set by spi controller drivers based on the "speed_hz"
> > number and they should chose a lower spi rate than 3110's baud rate
> > if can't find a same rate. For a spi controller support multiple
> > devices including 3110, the delay from 3110 is inevitable no matter
> > we use a explicit udelay or the dynamic spi rate.
> >
> > Till here, I have to say the "spi_transfer" of spi core is well
> > designed, which provides bits_per_word/delay_usecs/speed_hz of
> > great flexibility for developing spi device/controller drivers
> 
> You're abusing the speed_hz setting of the SPI controller.  By
> clocking down the SPI bus, you're effectively using the SPI completion
> interrupt as a high resolution timer to schedule your character
> transmissions so that you don't overrun the max3110 which has no tx
> fifo.  Using a udelay is also the wrong thing to do because it chews
> up CPU cycles to achieve the same result.
> 
> The correct thing to do is to either use a high resolution timer, or
> the IRQ from the MAX3110 itself to schedule your spi transfers, and
> set the SPI rate to the highest frequency that the MAX3110 can handle
> so that your driver neither hogs the SPI bus, nor wastes CPU cycles.
> You can improve the performance of the driver by using spi_async()
> instead of spi_sync() it enqueue the transfers so that you can handle
> everything at IRQ context and you don't need to drop into a workqueue
> to process the data.

Yes, using a high resolution timer instead of udelay is definitely better.
The reason we didn't use it is the delay for 115200 bps is about 90us,
which is shorter than the time of executing an hr_timer(IRQ handling and
precess waking stuff), but it's our HW's problem anyway.

But this doesn't help on buffer read/write, and only works for one word per
spi_transfer model as 3110 only has 1 word TX buffer and 8 word RX buffer.
We normally use such a test case, Ctrl+C a big string (500 Bytes), and Ctrl+V
it on the 3110 console, check if they are correctly received and echoed on
screen, one word per transfer can hardly make that, due to the cost in spi
layer and tty layer, while buffer read/write model can.

If "speed_hz" are not for this case, then where should it be used for,
spi core already has "max_speed_hz" specifying the maxim clock permitted by
HW. Using speed_hz does affect other devices connecting to the same controller,
but can we provide an option for the platform code to chose whether dynamic
lower spi clk is allowed, in case the spi controller has only 3110 as its
slave device.

Thanks,
Feng 
 
> 
> g.

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Grant Likely]

Hi Feng,

I'm really concerned with this driver.  I think it needs a lot more
work before it is ready for prime-time.  Again, you might want to
consider asking Greg to add it to the staging tree while you clean
stuff up and coordinate with the max3100 driver author.

Comments below.

On Thu, Mar 4, 2010 at 12:25 AM, Feng Tang <feng.tang@intel.com> wrote:
> Hi All,
>
> Here is a driver for Maxim 3110 SPI-UART device, please help to review.
>
> It has been validated with Designware SPI controller (drivers/spi: dw_spi.c &
> dw_spi_pci.c). It supports polling and IRQ mode, supports batch read, and
> provides a console.
>
> change log:
>  since v5:
>        * Make the spi data buffer DMA safe, thanks to Mssakazu Mokuno,
>          David Brownell and Grant Likely for pointing the bug out
>  since v4:
>        * Add explanation why not using DMA
>        * Fix a bug in max3110_read_multi()
>  since v3:
>        * Remove the Designware controller related stuff
>        * Remove some initialization code which should be done in the platform
>          setup code
>        * Add a missing change for drivers/serial/Makefile
>  since v2:
>        * Address comments from Andrew Morton
>        * Use test/set_bit to avoid race condition for SMP/preempt case
>        * Fix bug related with endian order
>  since v1:
>        * Address comments from Alan Cox
>        * Use a "use_irq" module parameter to runtime check whether
>          to use IRQ mode
>        * Add the suspend/resume implementation
>
> Thanks!
> Feng
>
>
> From 6ff1d75c34d9465d4ffce076350473bfaf5404a5 Mon Sep 17 00:00:00 2001
> From: Feng Tang <feng.tang@intel.com>
> Date: Fri, 26 Feb 2010 11:37:29 +0800
> Subject: [PATCH] serial: spi: add spi-uart driver for Maxim 3110
>
> This is the driver for Max3110 SPI-UART device, which connect
> to host with SPI interface. It supports baud rates from 300 to
> 230400, and supports both polling and IRQ mode, as well as
> providing a console for system use
>
> Its datasheet could be found here:
> http://datasheets.maxim-ic.com/en/ds/MAX3110E-MAX3111E.pdf
>
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: David Brownell <david-b@pacbell.net>
> Cc: Greg KH <greg@kroah.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Acked-by: Alan Cox <alan@linux.intel.com>
> ---
>  drivers/serial/Kconfig      |    8 +
>  drivers/serial/Makefile     |    1 +
>  drivers/serial/max3110.c    |  892 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/serial/max3110.h    |   61 +++

Why the separate .h file?  If it isn't used by multiple .c files, then
it belongs in the .c file itself.

>  include/linux/serial_core.h |    3 +
>  5 files changed, 965 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/serial/max3110.c
>  create mode 100644 drivers/serial/max3110.h
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 9ff47db..94aa282 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -688,6 +688,14 @@ config SERIAL_SA1100_CONSOLE
>          your boot loader (lilo or loadlin) about how to pass options to the
>          kernel at boot time.)
>
> +config SERIAL_MAX3110
> +       tristate "SPI UART driver for Max3110"
> +       depends on SPI_MASTER
> +       select SERIAL_CORE
> +       select SERIAL_CORE_CONSOLE
> +       help
> +         This is the UART protocol driver for MAX3110 device
> +
>  config SERIAL_BFIN
>        tristate "Blackfin serial port support"
>        depends on BLACKFIN
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index 5548fe7..b93d8a0 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_SERIAL_S3C24A0) += s3c24a0.o
>  obj-$(CONFIG_SERIAL_S3C6400) += s3c6400.o
>  obj-$(CONFIG_SERIAL_S5PC100) += s3c6400.o
>  obj-$(CONFIG_SERIAL_MAX3100) += max3100.o
> +obj-$(CONFIG_SERIAL_MAX3110) += max3110.o

NAK.  I've looked at the max3100 driver, and the max3110 data sheet
specifically states that it is max3100 compatible.  I'm opposed to the
merging of this driver until you've at least made an attempt at
working with the max3100 driver author to produce a single driver.

>  obj-$(CONFIG_SERIAL_IP22_ZILOG) += ip22zilog.o
>  obj-$(CONFIG_SERIAL_MUX) += mux.o
>  obj-$(CONFIG_SERIAL_68328) += 68328serial.o
> diff --git a/drivers/serial/max3110.c b/drivers/serial/max3110.c
> new file mode 100644
> index 0000000..9b39914
> --- /dev/null
> +++ b/drivers/serial/max3110.c
> @@ -0,0 +1,892 @@
> +/*
> + * max3110.c - spi uart protocol driver for Maxim 3110
> + *
> + * Copyright (c) 2009, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +/*
> + * Note:
> + * 1. From Max3110 spec, the Rx FIFO has 8 words, while the Tx FIFO only has
> + *    1 word. If SPI master controller doesn't support sclk frequency change,
> + *    then the char need be sent out one by one with some delay
> + *
> + * 2. Currently only RX availabe interrrupt is used
> + */
> +
> +#include <linux/module.h>
> +#include <linux/ioport.h>
> +#include <linux/init.h>
> +#include <linux/console.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial_reg.h>
> +#include <linux/kthread.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +
> +#include "max3110.h"
> +
> +#define PR_FMT "max3110: "
> +
> +struct uart_max3110 {
> +       struct uart_port port;
> +       struct spi_device *spi;
> +       char *name;
> +
> +       wait_queue_head_t wq;
> +       struct task_struct *main_thread;
> +       struct task_struct *read_thread;
> +       spinlock_t lock;
> +
> +       u32 baud;
> +       u16 cur_conf;
> +       u8 clock;
> +       u8 parity, word_7bits;
> +       u16 irq;
> +
> +       /* bit map for UART status */
> +       unsigned long flags;
> +#define M3110_CON_TX_NEED      0
> +#define M3110_UART_TX_NEED     1
> +#define M3110_IRQ_PENDING      2
> +       unsigned long mthread_up;
> +
> +       /* console buffer */
> +       struct circ_buf con_xmit;
> +};
> +
> +static struct uart_max3110 *pmax;

Don't do this.  To start, it limits the driver to only ever being able
to handle a single instance of the device.  Some drivers use a fixed
size table instead, but that isn't any better either.

Instead of relying on a global variable, store the struct uart_max3110
pointer inside the spi_device.dev.p private data pointer.

For the console portion use the struct console.data pointer.

> +
> +static int use_irq = 1;
> +module_param(use_irq, int, 0444);
> +MODULE_PARM_DESC(use_irq, "Whether using Max3110's IRQ capability");

Why is this a module param?  Shouldn't this be determined on whether
or not the drivers pdata structure is populated with an irq number?

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

Re: [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Feng Tang]

On Mon, 8 Mar 2010 12:30:13 +0800
Grant Likely <grant.likely@secretlab.ca> wrote:

> Hi Feng,
> 
> I'm really concerned with this driver.  I think it needs a lot more
> work before it is ready for prime-time.  Again, you might want to
> consider asking Greg to add it to the staging tree while you clean
> stuff up and coordinate with the max3100 driver author.
> 
> Comments below.

Hi Grant,

Thanks for the comments, will address them in future submission.

For your suggestion about merging with max3100.c, I did tried but failed
as the basic working model differ a lot, anyway I will recheck that.

- Feng 

[PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[christian pellegrin]

On Thu, Mar 4, 2010 at 8:22 AM,
<spi-devel-general-request-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> wrote:

>
> Hi All,
>

Hi, first of all let me apologize that I haven't see your previous
versions of the driver. I completely missed them. I am the author of
the max3100 driver. As far as I can see the max3110 is just a max3100
with the TTL to EIA level converter. So, first of all, may I ask why
you did start a new driver from scratch? I'm quite happy with the
current one, the only problem I see is that it uses worqueues which
are scheduled like normal processes and so under load their latency is
*big*. I wanted to move to threaded interrupts (otherwise you have to
use some ugly tricks to give workqueue threads real-time scheduling
class) but was waiting for my beta testers^H^H^H^H^H^H^H^H^H^H^H^H^H
customers to move to modern kernel that support them. Anyway the patch
is trivial and I can provide it if there is some interest. A quick
glance at your patch shows you are using the same sched_other threads
so you will face the same problems. I saw you added console capability
which isn't present in current driver, but it's easy to add (I didn't
implement it initially because I don't see a good idea using the
max3100 for initial bring-up of an embedded system because it relies
on the SPI subsystem, worqueues, scheduler and such complicated
things). Anyway keep in mind that the MAX3100 is rather low-end UART
so don't push it too much.

I'm more than happy to work with you to have just one perfect (or
almost perfect) driver instead of two of them half-done. See some
comments inline.

> + * 1. From Max3110 spec, the Rx FIFO has 8 words, while the Tx FIFO only has
> + *    1 word. If SPI master controller doesn't support sclk frequency change,
> + *    then the char need be sent out one by one with some delay

I don't think this is a good idea to change speed. Just check the T
bit to see when the TX buffer is empty and send at the maximum
available speed. So the usage of the SPI bus is better.

> + *
> + * 2. Currently only RX availabe interrrupt is used

yeah, this is one of the reasons why MAX31x0 sucks: when you change
configuration of interrupts the  RX buffer is cleared. So you cannot
activate/deactivate TX empty interrupt as needed.

> +static int use_irq = 1;
> +module_param(use_irq, int, 0444);
> +MODULE_PARM_DESC(use_irq, "Whether using Max3110's IRQ capability");

I don't think it's a good idea using an UART in polling mode, it just
kills the system. Just pretend this is not possible otherwise some
hardware guys will pester us to do this just to save an electric wire.

> +       memset(&x, 0, sizeof x);
> +       x.len = len;
> +       x.tx_buf = txbuf;
> +       x.rx_buf = rxbuf;

why don't initialize this like:

        struct spi_transfer x = {
                .tx_buf = txbuf,
                .rx_buf = rxbuf,
                .len = len,
        };


> +       buf = kmalloc(8, GFP_KERNEL | GFP_DMA);

you do kmalloc and kfree in routines that are called in quite tight
loops: I guess it's an overkill for performance.

> +static unsigned int serial_m3110_tx_empty(struct uart_port *port)
> +{
> +       return 1;
> +}
> +
> +static void serial_m3110_stop_tx(struct uart_port *port)
> +{
> +       return;
> +}
> +
> +static void serial_m3110_stop_rx(struct uart_port *port)
> +{
> +       return;
> +}

are you sure it's sane to just ignore these from higher level?

> +       u8 recv_buf[512], *pbuf;

is this sane to allocate 512 byte on the stack?

> +               wait_event_interruptible(*wq,
> +                               max->flags || kthread_should_stop());
> +               test_and_set_bit(0, &max->mthread_up);

I guess you are using mthread_up to avoid awakening the main thread
too many times. But this makes sense? Anyway because the awakening and
the setting of the bit is not atomic it won't work anyway.

> +/* Don't handle hw handshaking */
> +static unsigned int serial_m3110_get_mctrl(struct uart_port *port)
> +{
> +       return TIOCM_DSR | TIOCM_CAR | TIOCM_DSR;
> +}
> +
> +static void serial_m3110_set_mctrl(struct uart_port *port, unsigned int mctrl)
> +{
> +}
> +

this is quite bad because the MAX3100 is rather slow.

Bye!

-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Feng Tang]

Hi Christian,

Thanks for taking time to review the driver. Please see my answers inline

- Feng
On Wed, 17 Mar 2010 01:22:46 +0800
christian pellegrin <chripell@gmail.com> wrote:

> On Thu, Mar 4, 2010 at 8:22 AM,
> <spi-devel-general-request@lists.sourceforge.net> wrote:
> 
> >
> > Hi All,
> >
> 
> Hi, first of all let me apologize that I haven't see your previous
> versions of the driver. I completely missed them. I am the author of
> the max3100 driver. As far as I can see the max3110 is just a max3100
> with the TTL to EIA level converter. So, first of all, may I ask why
> you did start a new driver from scratch? I'm quite happy with the
I don't want to reinvent the wheel, as I have many more tasks to handle :)
My 3110 driver started to work in 2008(surely very basic at that time) before
your driver is posted on public. And the reasons I still posted driver here are:
1. It provides a console
2. It use buffer read/write to meet our performance goal, max3110 doesn't have
good FIFO, but spi controller have and we can leverage that for performance 

If we can cooperate to make current 3100 driver meet our expectation, I'm more
than happy to drop my 3110 one. I'd rather submit some bug fix for it than
posting 6 or more versions of code :)

> current one, the only problem I see is that it uses worqueues which
> are scheduled like normal processes and so under load their latency is
> *big*. I wanted to move to threaded interrupts (otherwise you have to
> use some ugly tricks to give workqueue threads real-time scheduling
> class) but was waiting for my beta testers^H^H^H^H^H^H^H^H^H^H^H^H^H
> customers to move to modern kernel that support them. Anyway the patch
> is trivial and I can provide it if there is some interest. A quick
> glance at your patch shows you are using the same sched_other threads
> so you will face the same problems. I saw you added console capability
> which isn't present in current driver, but it's easy to add (I didn't
> implement it initially because I don't see a good idea using the
> max3100 for initial bring-up of an embedded system because it relies
> on the SPI subsystem, worqueues, scheduler and such complicated
> things). Anyway keep in mind that the MAX3100 is rather low-end UART
> so don't push it too much.
It's not me push it too much, but I was pushed too much :)
We can't make the decision how the HW is used on all kinds of platforms. For
ours, we heavily rely on it as a sole console, and basic requirement is:
the datasheet say it supports 230400~300 baud rate, then driver guy need make
it work as it claims, like copying 500 bytes string somewhere and paste it on
this console in 230400/600 rate.

For the kernel early boot phase debug, I actually had another 3110
early_printk patch which can work right after the kernel starts

> 
> I'm more than happy to work with you to have just one perfect (or
> almost perfect) driver instead of two of them half-done. See some
> comments inline.
Can't agree more, all I want is just a upstream driver which meets the basic
requirement.

> 
> > + * 1. From Max3110 spec, the Rx FIFO has 8 words, while the Tx
> > FIFO only has
> > + *    1 word. If SPI master controller doesn't support sclk
> > frequency change,
> > + *    then the char need be sent out one by one with some delay
> 
> I don't think this is a good idea to change speed. Just check the T
> bit to see when the TX buffer is empty and send at the maximum
> available speed. So the usage of the SPI bus is better.
Yes, checking T bit is a good idea for single word transfer model, but it
doesn't help when using buffer read/write

> 
> > + *
> > + * 2. Currently only RX availabe interrrupt is used
> 
> yeah, this is one of the reasons why MAX31x0 sucks: when you change
> configuration of interrupts the  RX buffer is cleared. So you cannot
> activate/deactivate TX empty interrupt as needed.
> 
> > +static int use_irq = 1;
> > +module_param(use_irq, int, 0444);
> > +MODULE_PARM_DESC(use_irq, "Whether using Max3110's IRQ
> > capability");
> 
> I don't think it's a good idea using an UART in polling mode, it just
> kills the system. Just pretend this is not possible otherwise some
> hardware guys will pester us to do this just to save an electric wire.
Yes, like Grant and others have mentioned, I plan to use spi->irq as the
condition, platform code which initialize the board_info should clear the
irq to 0 if they won't use IRQ for 3110

	if (spi->irq)
		request_irq(spi->irq,...);
	else
		start_read_thread;

> 
> 
> > +       buf = kmalloc(8, GFP_KERNEL | GFP_DMA);
> 
> you do kmalloc and kfree in routines that are called in quite tight
> loops: I guess it's an overkill for performance.

Overkill is just the word I have used when replying comments on my
earlier version :)

I didn't use kmalloc/kfree in my earlier submission, but other developers
mentioned the buffer allocated here need be DMA safe, as 3110 may work
with many kinds of SPI controller, some of them only supports DMA mode.

> 
> are you sure it's sane to just ignore these from higher level?
> 
> > +       u8 recv_buf[512], *pbuf;
> 
> is this sane to allocate 512 byte on the stack?

The function is called only in driver's own main thread and reader thread
context, 512 should be safe enough. Also the reason I don't use
kzalloc/kfree is the same as you mentioned: performance

> 
> > +               wait_event_interruptible(*wq,
> > +                               max->flags ||
> > kthread_should_stop());
> > +               test_and_set_bit(0, &max->mthread_up);
> 
> I guess you are using mthread_up to avoid awakening the main thread
> too many times. But this makes sense? Anyway because the awakening and
> the setting of the bit is not atomic it won't work anyway.
Why this can't work? the test/set_bit are atomic operations. This bit
check is not exclusive, it just try to save some waking up, though waking a
already running thread doesn't cost too much as mentioned by Andrew

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

Re: [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[christian pellegrin]

On Wed, Mar 17, 2010 at 3:35 AM, Feng Tang <feng.tang@intel.com> wrote:
> Hi Christian,

Hi,

> your driver is posted on public. And the reasons I still posted driver here are:
> 1. It provides a console

I'll provide a patch to the mainline driver for the console. Let me
just finish my daily job duties ;-)

> 2. It use buffer read/write to meet our performance goal, max3110 doesn't have
> good FIFO, but spi controller have and we can leverage that for performance

I think this line of thinking is wrong. SPI generic layer is well ....
generic. For example the platform I use is the S3C24xx which has a SPI
shift register depth of just one byte. And using DMA is not an option
because the setup of a DMA transfer takes tens of microseconds.
Additionally the SPI driver in mainline is based on bit-banging which
uses workqueues and so is limited by the fact that it fights with
other sched_other processes. Unfortunately an alternative driver which
was posted never got mainlined and we, who need it, have to maintain
it ourself. But this is another story, the bottom line is that we have
to cope with the most basic SPI support we can meet.

This said I tested the throughput of the driver by doing a zmodem
transfer and, as far as I remember, the results were pretty good.
Please note that the transmission is already buffered in higher layer.
Don't be fooled by the fact that tx_empty is correctly implemented in
the actual driver: you *have* to implement it otherwise some things
like termios call tcdrain() will be broken. For example applications
that do RS485 rx/tx switching in user-space will go nuts without it.
As far receiving is concerned .....

> the datasheet say it supports 230400~300 baud rate, then driver guy need make
> it work as it claims, like copying 500 bytes string somewhere and paste it on
> this console in 230400/600 rate.

.... this is exactly the problem I was facing: loosing chars on RX. I
tracked the problem to a too long latency in awakening of the
workqueue (the RX FIFO of the MAX3100 overflows). I will post shortly
a patch that moves this to threaded interrupts which solved the
problem on the S3C platform I mentioned above working at 115200. A
quick and dirty fix for workqueues is posted here:
http://www.evolware.org/chri/paciugo/index.html. Anyway it has to do
with scheduling of processes so many more factors are concerned, like
HZ and preemption being enabled.

I will post some tests together with the patch, if you have some
benchmarks you like to be run don't hesitate to send them. Thanks in
advance!

>> I'm more than happy to work with you to have just one perfect (or
>> almost perfect) driver instead of two of them half-done. See some
>> comments inline.
> Can't agree more, all I want is just a upstream driver which meets the basic
> requirement.
>

I think upstream drivers have to meet the requirement of the average
user not be tailored to specific user-cases (see the Android code
inclusion debate). For example changing SPI speed to meet some rate of
data flow is wrong. Nothing will assure you that you get the rate you
asked: it all depends of the master SPI clock and the granularity
resulting by its division. So I still think the best way is poll the T
bit by using the SPI bus as little as possible.

>> I don't think this is a good idea to change speed. Just check the T
>> bit to see when the TX buffer is empty and send at the maximum
>> available speed. So the usage of the SPI bus is better.
> Yes, checking T bit is a good idea for single word transfer model, but it
> doesn't help when using buffer read/write

I really don't understand why, can you elaborate on this?

> I didn't use kmalloc/kfree in my earlier submission, but other developers
> mentioned the buffer allocated here need be DMA safe, as 3110 may work
> with many kinds of SPI controller, some of them only supports DMA mode.
>

why you don't preallocate buffer for SPI transfers that are DMA-safe?
For example the mcp251x driver
(http://lxr.free-electrons.com/source/drivers/net/can/mcp251x.c?v=2.6.34-rc1)
does this.

>> I guess you are using mthread_up to avoid awakening the main thread
>> too many times. But this makes sense? Anyway because the awakening and
>> the setting of the bit is not atomic it won't work anyway.
> Why this can't work? the test/set_bit are atomic operations. This bit
> check is not exclusive, it just try to save some waking up, though waking a
> already running thread doesn't cost too much as mentioned by Andrew

First of all I think that the cost of re-awakening an already running
thread is lower than the logic of testing bits. My note was just
nit-picking: there is a window between when the task was woken-up and
where you set the bit, so it doesn't guarantee you that you aren't
awakening the thread more than once anyway.

Bye,

PS.: sorry to have missed you previous emails where some points where
already discussed. Please CC me next time, unfortunately spi-devel
mailing list carries quite a bit of spam and so I have to search its
post in the trash folder. I realized this is the reason why I didn't
see your previous posts :-/

-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."

Re: [PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Feng Tang]

On Wed, 17 Mar 2010 15:39:23 +0800
christian pellegrin <chripell@gmail.com> wrote:

> On Wed, Mar 17, 2010 at 3:35 AM, Feng Tang <feng.tang@intel.com>
> wrote:
> > Hi Christian,
> 
> Hi,
> 
> > your driver is posted on public. And the reasons I still posted
> > driver here are: 1. It provides a console
> 
> I'll provide a patch to the mainline driver for the console. Let me
> just finish my daily job duties ;-)

Cool!! I can test that on my platform.

> 
> > 2. It use buffer read/write to meet our performance goal, max3110
> > doesn't have good FIFO, but spi controller have and we can leverage
> > that for performance
> 
> I think this line of thinking is wrong. SPI generic layer is well ....
> generic. For example the platform I use is the S3C24xx which has a SPI
> shift register depth of just one byte. And using DMA is not an option
> because the setup of a DMA transfer takes tens of microseconds.
> Additionally the SPI driver in mainline is based on bit-banging which
> uses workqueues and so is limited by the fact that it fights with
> other sched_other processes. Unfortunately an alternative driver which
> was posted never got mainlined and we, who need it, have to maintain
> it ourself. But this is another story, the bottom line is that we have
> to cope with the most basic SPI support we can meet.

I'm no your side suggesting not to use DMA for 3110/3100, but David and
others have a valid point, we don't know what kind of SPI controller that
this driver will work with, and the driver has to be as general and
compatible as possible.

> 
> .... this is exactly the problem I was facing: loosing chars on RX. I
> tracked the problem to a too long latency in awakening of the
> workqueue (the RX FIFO of the MAX3100 overflows). I will post shortly
> a patch that moves this to threaded interrupts which solved the
> problem on the S3C platform I mentioned above working at 115200. A
> quick and dirty fix for workqueues is posted here:
> http://www.evolware.org/chri/paciugo/index.html. Anyway it has to do
> with scheduling of processes so many more factors are concerned, like
> HZ and preemption being enabled.
> 
> I will post some tests together with the patch, if you have some
> benchmarks you like to be run don't hesitate to send them. Thanks in
> advance!

heh, I don't have benchmarks but simply the copy/paste stuff and using
zmodem sometimes.
> 
> >> I don't think this is a good idea to change speed. Just check the T
> >> bit to see when the TX buffer is empty and send at the maximum
> >> available speed. So the usage of the SPI bus is better.
> > Yes, checking T bit is a good idea for single word transfer model,
> > but it doesn't help when using buffer read/write
> 
> I really don't understand why, can you elaborate on this?

Checking T bit has to be done in word level, send one word out, read one
back and check T bit inside one spi xfer.

Why not use the buffer model, the HW has a 8 words RX buffer, why not read
8 words back in one spi xfer, saving a lot of system cost. And if we make
the spi clk slightly slower than the baud rate, there will be no TX
overflow problem. 

> 
> > I didn't use kmalloc/kfree in my earlier submission, but other
> > developers mentioned the buffer allocated here need be DMA safe, as
> > 3110 may work with many kinds of SPI controller, some of them only
> > supports DMA mode.
> >
> 
> why you don't preallocate buffer for SPI transfers that are DMA-safe?
> For example the mcp251x driver
> (http://lxr.free-electrons.com/source/drivers/net/can/mcp251x.c?v=2.6.34-rc1)
> does this.
Some function will be called in several places, say re-entered, hard to use
a preallocate buffer.

> 
> >> I guess you are using mthread_up to avoid awakening the main thread
> >> too many times. But this makes sense? Anyway because the awakening
> >> and the setting of the bit is not atomic it won't work anyway.
> > Why this can't work? the test/set_bit are atomic operations. This
> > bit check is not exclusive, it just try to save some waking up,
> > though waking a already running thread doesn't cost too much as
> > mentioned by Andrew
> 
> First of all I think that the cost of re-awakening an already running
> thread is lower than the logic of testing bits. My note was just
> nit-picking: there is a window between when the task was woken-up and
> where you set the bit, so it doesn't guarantee you that you aren't
> awakening the thread more than once anyway.

OK, got your point now. test/set_bit's cost depends on the platform, 
and my simple test using perf tool showing it cost 10us level to wake
a running thread.
Anyway, re-wake it does no harm :)

Thanks,
Feng

[PATCH v6] serial: spi: add spi-uart driver for Maxim 3110

[Feng Tang]

Hi All,

Here is a driver for Maxim 3110 SPI-UART device, please help to review.

It has been validated with Designware SPI controller (drivers/spi: dw_spi.c &
dw_spi_pci.c). It supports polling and IRQ mode, supports batch read, and
provides a console.

change log:
 since v5:
        * Make the spi data buffer DMA safe, thanks to Mssakazu Mokuno, 
          David Brownell and Grant Likely for pointing the bug out
 since v4:
        * Add explanation why not using DMA
        * Fix a bug in max3110_read_multi()
 since v3:
        * Remove the Designware controller related stuff
        * Remove some initialization code which should be done in the platform
          setup code
        * Add a missing change for drivers/serial/Makefile
 since v2:
        * Address comments from Andrew Morton
        * Use test/set_bit to avoid race condition for SMP/preempt case
        * Fix bug related with endian order
 since v1:
        * Address comments from Alan Cox
        * Use a "use_irq" module parameter to runtime check whether
          to use IRQ mode
        * Add the suspend/resume implementation

Thanks!
Feng


>From 6ff1d75c34d9465d4ffce076350473bfaf5404a5 Mon Sep 17 00:00:00 2001
From: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Fri, 26 Feb 2010 11:37:29 +0800
Subject: [PATCH] serial: spi: add spi-uart driver for Maxim 3110

This is the driver for Max3110 SPI-UART device, which connect
to host with SPI interface. It supports baud rates from 300 to
230400, and supports both polling and IRQ mode, as well as
providing a console for system use

Its datasheet could be found here:
http://datasheets.maxim-ic.com/en/ds/MAX3110E-MAX3111E.pdf

Signed-off-by: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
Cc: Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Acked-by: Alan Cox <alan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/serial/Kconfig      |    8 +
 drivers/serial/Makefile     |    1 +
 drivers/serial/max3110.c    |  892 +++++++++++++++++++++++++++++++++++++++++++
 drivers/serial/max3110.h    |   61 +++
 include/linux/serial_core.h |    3 +
 5 files changed, 965 insertions(+), 0 deletions(-)
 create mode 100644 drivers/serial/max3110.c
 create mode 100644 drivers/serial/max3110.h

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 9ff47db..94aa282 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -688,6 +688,14 @@ config SERIAL_SA1100_CONSOLE
 	  your boot loader (lilo or loadlin) about how to pass options to the
 	  kernel at boot time.)
 
+config SERIAL_MAX3110
+	tristate "SPI UART driver for Max3110"
+	depends on SPI_MASTER
+	select SERIAL_CORE
+	select SERIAL_CORE_CONSOLE
+	help
+	  This is the UART protocol driver for MAX3110 device
+
 config SERIAL_BFIN
 	tristate "Blackfin serial port support"
 	depends on BLACKFIN
diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index 5548fe7..b93d8a0 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_SERIAL_S3C24A0) += s3c24a0.o
 obj-$(CONFIG_SERIAL_S3C6400) += s3c6400.o
 obj-$(CONFIG_SERIAL_S5PC100) += s3c6400.o
 obj-$(CONFIG_SERIAL_MAX3100) += max3100.o
+obj-$(CONFIG_SERIAL_MAX3110) += max3110.o
 obj-$(CONFIG_SERIAL_IP22_ZILOG) += ip22zilog.o
 obj-$(CONFIG_SERIAL_MUX) += mux.o
 obj-$(CONFIG_SERIAL_68328) += 68328serial.o
diff --git a/drivers/serial/max3110.c b/drivers/serial/max3110.c
new file mode 100644
index 0000000..9b39914
--- /dev/null
+++ b/drivers/serial/max3110.c
@@ -0,0 +1,892 @@
+/*
+ * max3110.c - spi uart protocol driver for Maxim 3110
+ *
+ * Copyright (c) 2009, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+/*
+ * Note:
+ * 1. From Max3110 spec, the Rx FIFO has 8 words, while the Tx FIFO only has
+ *    1 word. If SPI master controller doesn't support sclk frequency change,
+ *    then the char need be sent out one by one with some delay
+ *
+ * 2. Currently only RX availabe interrrupt is used
+ */
+
+#include <linux/module.h>
+#include <linux/ioport.h>
+#include <linux/init.h>
+#include <linux/console.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+#include <linux/serial_core.h>
+#include <linux/serial_reg.h>
+#include <linux/kthread.h>
+#include <linux/delay.h>
+#include <linux/spi/spi.h>
+
+#include "max3110.h"
+
+#define PR_FMT	"max3110: "
+
+struct uart_max3110 {
+	struct uart_port port;
+	struct spi_device *spi;
+	char *name;
+
+	wait_queue_head_t wq;
+	struct task_struct *main_thread;
+	struct task_struct *read_thread;
+	spinlock_t lock;
+
+	u32 baud;
+	u16 cur_conf;
+	u8 clock;
+	u8 parity, word_7bits;
+	u16 irq;
+
+	/* bit map for UART status */
+	unsigned long flags;
+#define M3110_CON_TX_NEED	0
+#define M3110_UART_TX_NEED	1
+#define M3110_IRQ_PENDING	2
+	unsigned long mthread_up;
+
+	/* console buffer */
+	struct circ_buf con_xmit;
+};
+
+static struct uart_max3110 *pmax;
+
+static int use_irq = 1;
+module_param(use_irq, int, 0444);
+MODULE_PARM_DESC(use_irq, "Whether using Max3110's IRQ capability");
+
+static void receive_chars(struct uart_max3110 *max,
+				unsigned char *str, int len);
+static int max3110_read_multi(struct uart_max3110 *max, u8 *buf);
+static void max3110_con_receive(struct uart_max3110 *max);
+
+static int max3110_write_then_read(struct uart_max3110 *max,
+		const void *txbuf, void *rxbuf, unsigned len, int always_fast)
+{
+	struct spi_device *spi = max->spi;
+	struct spi_message	message;
+	struct spi_transfer	x;
+	int ret;
+
+	spi_message_init(&message);
+	memset(&x, 0, sizeof x);
+	x.len = len;
+	x.tx_buf = txbuf;
+	x.rx_buf = rxbuf;
+	spi_message_add_tail(&x, &message);
+
+	if (always_fast)
+		x.speed_hz = spi->max_speed_hz;
+	else if (max->baud)
+		x.speed_hz = max->baud;
+
+	/* Do the i/o */
+	ret = spi_sync(spi, &message);
+	return ret;
+}
+
+/* Write a 16b word to the device */
+static int max3110_out(struct uart_max3110 *max, const u16 out)
+{
+	void *buf;
+	u16 *obuf, *ibuf;
+	u8  ch;
+	int ret;
+
+	buf = kmalloc(8, GFP_KERNEL | GFP_DMA);
+	if (!buf)
+		return -ENOMEM;
+
+	obuf = buf;
+	ibuf = buf + 4;
+	*obuf = out;
+	ret = max3110_write_then_read(max, obuf, ibuf, 2, 1);
+	if (ret) {
+		pr_warning(PR_FMT "%s(): get err msg %d when sending 0x%x\n",
+				__func__, ret, out);
+		goto exit;
+	}
+
+	/* If some valid data is read back */
+	if (*ibuf & MAX3110_READ_DATA_AVAILABLE) {
+		ch = *ibuf & 0xff;
+		receive_chars(max, &ch, 1);
+	}
+
+exit:
+	kfree(buf);
+	return ret;
+}
+
+/*
+ * This is usually used to read data from SPIC RX FIFO, which doesn't
+ * need any delay like flushing character out.
+ *
+ * Return how many valide bytes are read back
+ */
+static int max3110_read_multi(struct uart_max3110 *max, u8 *rxbuf)
+{
+	void *buf;
+	u16 *obuf, *ibuf;
+	u8 *pbuf, valid_str[M3110_RX_FIFO_DEPTH];
+	int i, j, blen;
+
+	blen = M3110_RX_FIFO_DEPTH * sizeof(u16);
+	buf = kmalloc(blen * 2, GFP_KERNEL | GFP_DMA);
+	if (!buf) {
+		pr_warning(PR_FMT "%s(): fail to alloc dma buffer\n", __func__);
+		return 0;
+	}
+
+	/* tx/rx always have the same length */
+	memset(buf, 0, blen * 2);
+	obuf = buf;
+	ibuf = buf + blen;
+
+	if (max3110_write_then_read(max, obuf, ibuf, blen, 1)) {
+		kfree(buf);
+		return 0;
+	}
+
+	/* If caller doesn't provide a buffer, then handle received char */
+	pbuf = rxbuf ? rxbuf : valid_str;
+
+	for (i = 0, j = 0; i < M3110_RX_FIFO_DEPTH; i++) {
+		if (ibuf[i] & MAX3110_READ_DATA_AVAILABLE)
+			pbuf[j++] = ibuf[i] & 0xff;
+	}
+
+	if (j && (pbuf == valid_str))
+		receive_chars(max, valid_str, j);
+
+	kfree(buf);
+	return j;
+}
+
+static void serial_m3110_con_putchar(struct uart_port *port, int ch)
+{
+	struct uart_max3110 *max =
+		container_of(port, struct uart_max3110, port);
+	struct circ_buf *xmit = &max->con_xmit;
+
+	if (uart_circ_chars_free(xmit)) {
+		xmit->buf[xmit->head] = (char)ch;
+		xmit->head = (xmit->head + 1) & (PAGE_SIZE - 1);
+	}
+}
+
+/*
+ * Print a string to the serial port trying not to disturb
+ * any possible real use of the port...
+ *
+ *	The console_lock must be held when we get here.
+ */
+static void serial_m3110_con_write(struct console *co,
+				const char *s, unsigned int count)
+{
+	if (!pmax)
+		return;
+
+	uart_console_write(&pmax->port, s, count, serial_m3110_con_putchar);
+
+	set_bit(M3110_CON_TX_NEED, &pmax->flags);
+	if (!test_bit(0, &pmax->mthread_up))
+		wake_up_process(pmax->main_thread);
+}
+
+static int __init
+serial_m3110_con_setup(struct console *co, char *options)
+{
+	struct uart_max3110 *max = pmax;
+	int baud = 115200;
+	int bits = 8;
+	int parity = 'n';
+	int flow = 'n';
+
+	pr_info(PR_FMT "setting up console\n");
+
+	if (!max) {
+		pr_err(PR_FMT "pmax is NULL, return");
+		return -ENODEV;
+	}
+
+	if (options)
+		uart_parse_options(options, &baud, &parity, &bits, &flow);
+
+	return uart_set_options(&max->port, co, baud, parity, bits, flow);
+}
+
+static struct tty_driver *serial_m3110_con_device(struct console *co,
+							int *index)
+{
+	struct uart_driver *p = co->data;
+	*index = co->index;
+	return p->tty_driver;
+}
+
+static struct uart_driver serial_m3110_reg;
+static struct console serial_m3110_console = {
+	.name		= "ttyS",
+	.write		= serial_m3110_con_write,
+	.device		= serial_m3110_con_device,
+	.setup		= serial_m3110_con_setup,
+	.flags		= CON_PRINTBUFFER,
+	.index		= -1,
+	.data		= &serial_m3110_reg,
+};
+
+
+static unsigned int serial_m3110_tx_empty(struct uart_port *port)
+{
+	return 1;
+}
+
+static void serial_m3110_stop_tx(struct uart_port *port)
+{
+	return;
+}
+
+static void serial_m3110_stop_rx(struct uart_port *port)
+{
+	return;
+}
+
+#define WORDS_PER_XFER	128
+static void send_circ_buf(struct uart_max3110 *max,
+				struct circ_buf *xmit)
+{
+	void *buf;
+	u16 *obuf, *ibuf;
+	u8 valid_str[WORDS_PER_XFER];
+	int i, j, len, blen, dma_size, left, ret = 0;
+
+
+	dma_size = WORDS_PER_XFER * sizeof(u16) * 2;
+	buf = kmalloc(dma_size, GFP_KERNEL | GFP_DMA);
+	if (!buf)
+		return;
+	obuf = buf;
+	ibuf = buf + dma_size/2;
+
+	while (!uart_circ_empty(xmit)) {
+		left = uart_circ_chars_pending(xmit);
+		while (left) {
+			len = min(left, WORDS_PER_XFER);
+			blen = len * sizeof(u16);
+			memset(obuf, 0, blen);
+			memset(ibuf, 0, blen);
+
+			for (i = 0; i < len; i++) {
+				obuf[i] = (u8)xmit->buf[xmit->tail] | WD_TAG;
+				xmit->tail = (xmit->tail + 1) &
+						(UART_XMIT_SIZE - 1);
+			}
+
+			/* Fail to send msg to console is not very critical */
+			ret = max3110_write_then_read(max, obuf, ibuf, blen, 0);
+			if (ret)
+				pr_warning(PR_FMT "%s(): get err msg %d\n",
+						__func__, ret);
+
+			for (i = 0, j = 0; i < len; i++) {
+				if (ibuf[i] & MAX3110_READ_DATA_AVAILABLE)
+					valid_str[j++] = ibuf[i] & 0xff;
+			}
+
+			if (j)
+				receive_chars(max, valid_str, j);
+
+			max->port.icount.tx += len;
+			left -= len;
+		}
+	}
+
+	kfree(buf);
+}
+
+static void transmit_char(struct uart_max3110 *max)
+{
+	struct uart_port *port = &max->port;
+	struct circ_buf *xmit = &port->state->xmit;
+
+	if (uart_circ_empty(xmit) || uart_tx_stopped(port))
+		return;
+
+	send_circ_buf(max, xmit);
+
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+		uart_write_wakeup(port);
+}
+
+/*
+ * This will be called by uart_write() and tty_write, can't
+ * go to sleep
+ */
+static void serial_m3110_start_tx(struct uart_port *port)
+{
+	struct uart_max3110 *max =
+		container_of(port, struct uart_max3110, port);
+
+	set_bit(M3110_UART_TX_NEED, &max->flags);
+	if (!test_bit(0, &max->mthread_up))
+		wake_up_process(max->main_thread);
+}
+
+static void receive_chars(struct uart_max3110 *max, unsigned char *str, int len)
+{
+	struct uart_port *port = &max->port;
+	struct tty_struct *tty;
+	int usable;
+
+	/* If uart is not opened, just return */
+	if (!port->state)
+		return;
+
+	tty = port->state->port.tty;
+	if (!tty)
+		return;
+
+	while (len) {
+		usable = tty_buffer_request_room(tty, len);
+		if (usable) {
+			tty_insert_flip_string(tty, str, usable);
+			str += usable;
+			port->icount.rx += usable;
+		}
+		len -= usable;
+	}
+	tty_flip_buffer_push(tty);
+}
+
+/*
+ * This routine will be used in read_thread or RX IRQ handling,
+ * it will first do one round buffer read(8 words), if there is some
+ * valid RX data, will try to read 5 more rounds till all data
+ * is read out.
+ *
+ * Use stack space as data buffer to save some system load, and chose
+ * 504 Btyes as a threadhold to do a bulk push to upper tty layer when
+ * receiving bulk data, a much bigger buffer may cause stack overflow
+ */
+static void max3110_con_receive(struct uart_max3110 *max)
+{
+	int loop = 1, num, total = 0;
+	u8 recv_buf[512], *pbuf;
+
+	pbuf = recv_buf;
+	do {
+		num = max3110_read_multi(max, pbuf);
+
+		if (num) {
+			loop = 5;
+			pbuf += num;
+			total += num;
+
+			if (total >= 504) {
+				receive_chars(max, recv_buf, total);
+				pbuf = recv_buf;
+				total = 0;
+			}
+		}
+	} while (--loop);
+
+	if (total)
+		receive_chars(max, recv_buf, total);
+}
+
+static int max3110_main_thread(void *_max)
+{
+	struct uart_max3110 *max = _max;
+	wait_queue_head_t *wq = &max->wq;
+	int ret = 0;
+	struct circ_buf *xmit = &max->con_xmit;
+
+	init_waitqueue_head(wq);
+	pr_info(PR_FMT "start main thread\n");
+
+	do {
+		wait_event_interruptible(*wq,
+				max->flags || kthread_should_stop());
+		test_and_set_bit(0, &max->mthread_up);
+
+		if (use_irq && test_bit(M3110_IRQ_PENDING, &max->flags)) {
+			max3110_con_receive(max);
+			clear_bit(M3110_IRQ_PENDING, &max->flags);
+		}
+
+		/* First handle console output */
+		if (test_bit(M3110_CON_TX_NEED, &max->flags)) {
+			send_circ_buf(max, xmit);
+			clear_bit(M3110_CON_TX_NEED, &max->flags);
+		}
+
+		/* Handle uart output */
+		if (test_bit(M3110_UART_TX_NEED, &max->flags)) {
+			transmit_char(max);
+			clear_bit(M3110_UART_TX_NEED, &max->flags);
+		}
+		test_and_clear_bit(0, &max->mthread_up);
+	} while (!kthread_should_stop());
+
+	return ret;
+}
+
+static irqreturn_t serial_m3110_irq(int irq, void *dev_id)
+{
+	struct uart_max3110 *max = dev_id;
+
+	/* max3110's irq is a falling edge, not level triggered,
+	 * so no need to disable the irq */
+	set_bit(M3110_IRQ_PENDING, &max->flags);
+
+	if (!test_bit(0, &max->mthread_up))
+		wake_up_process(max->main_thread);
+
+	return IRQ_HANDLED;
+}
+
+/* If don't use RX IRQ, then need a thread to polling read */
+static int max3110_read_thread(void *_max)
+{
+	struct uart_max3110 *max = _max;
+
+	pr_info(PR_FMT "start read thread\n");
+	do {
+		if (!test_bit(0, &max->mthread_up))
+			max3110_con_receive(max);
+
+		schedule_timeout_interruptible(HZ / 20);
+	} while (!kthread_should_stop());
+
+	return 0;
+}
+
+static int serial_m3110_startup(struct uart_port *port)
+{
+	struct uart_max3110 *max =
+		container_of(port, struct uart_max3110, port);
+	u16 config = 0;
+	int ret = 0;
+
+	if (port->line != 0) {
+		pr_err(PR_FMT "uart port startup failed\n");
+		return -1;
+	}
+
+	/* Disable all IRQ and config it to 115200, 8n1 */
+	config = WC_TAG | WC_FIFO_ENABLE
+			| WC_1_STOPBITS
+			| WC_8BIT_WORD
+			| WC_BAUD_DR2;
+
+	/* As we use thread to handle tx/rx, need set low latency */
+	port->state->port.tty->low_latency = 1;
+
+	if (use_irq) {
+		ret = request_irq(max->irq, serial_m3110_irq,
+					IRQ_TYPE_EDGE_FALLING, "max3110", max);
+		if (ret)
+			return ret;
+
+		/* Enable RX IRQ only */
+		config |= WC_RXA_IRQ_ENABLE;
+	} else {
+		/* If IRQ is disabled, start a read thread for input data */
+		max->read_thread =
+			kthread_run(max3110_read_thread, max, "max3110_read");
+		if (IS_ERR(max->read_thread)) {
+			ret = PTR_ERR(max->read_thread);
+			max->read_thread = 0;
+			pr_err(PR_FMT "Can't create read thread!");
+			return ret;
+		}
+	}
+
+	ret = max3110_out(max, config);
+	if (ret) {
+		if (use_irq)
+			free_irq(max->irq, max);
+		else
+			kthread_stop(max->read_thread);
+
+		return ret;
+	}
+
+	max->cur_conf = config;
+	return 0;
+}
+
+static void serial_m3110_shutdown(struct uart_port *port)
+{
+	struct uart_max3110 *max =
+		container_of(port, struct uart_max3110, port);
+	u16 config;
+
+	if (max->read_thread) {
+		kthread_stop(max->read_thread);
+		max->read_thread = NULL;
+	}
+
+	if (use_irq)
+		free_irq(max->irq, max);
+
+	/* Disable interrupts from this port */
+	config = WC_TAG | WC_SW_SHDI;
+	max3110_out(max, config);
+}
+
+static void serial_m3110_release_port(struct uart_port *port)
+{
+}
+
+static int serial_m3110_request_port(struct uart_port *port)
+{
+	return 0;
+}
+
+static void serial_m3110_config_port(struct uart_port *port, int flags)
+{
+	port->type = PORT_MAX3110;
+}
+
+static int
+serial_m3110_verify_port(struct uart_port *port, struct serial_struct *ser)
+{
+	/* We don't want the core code to modify any port params */
+	return -EINVAL;
+}
+
+
+static const char *serial_m3110_type(struct uart_port *port)
+{
+	struct uart_max3110 *max =
+		container_of(port, struct uart_max3110, port);
+	return max->name;
+}
+
+static void
+serial_m3110_set_termios(struct uart_port *port, struct ktermios *termios,
+		       struct ktermios *old)
+{
+	struct uart_max3110 *max =
+		container_of(port, struct uart_max3110, port);
+	unsigned char cval;
+	unsigned int baud, parity = 0;
+	int clk_div = -1;
+	u16 new_conf = max->cur_conf;
+
+	switch (termios->c_cflag & CSIZE) {
+	case CS7:
+		cval = UART_LCR_WLEN7;
+		new_conf |= WC_7BIT_WORD;
+		break;
+	default:
+		/* We only support CS7 & CS8 */
+		termios->c_cflag &= ~CSIZE;
+		termios->c_cflag |= CS8;
+	case CS8:
+		cval = UART_LCR_WLEN8;
+		new_conf |= WC_8BIT_WORD;
+		break;
+	}
+
+	baud = uart_get_baud_rate(port, termios, old, 0, 230400);
+
+	/* First calc the div for 1.8MHZ clock case */
+	switch (baud) {
+	case 300:
+		clk_div = WC_BAUD_DR384;
+		break;
+	case 600:
+		clk_div = WC_BAUD_DR192;
+		break;
+	case 1200:
+		clk_div = WC_BAUD_DR96;
+		break;
+	case 2400:
+		clk_div = WC_BAUD_DR48;
+		break;
+	case 4800:
+		clk_div = WC_BAUD_DR24;
+		break;
+	case 9600:
+		clk_div = WC_BAUD_DR12;
+		break;
+	case 19200:
+		clk_div = WC_BAUD_DR6;
+		break;
+	case 38400:
+		clk_div = WC_BAUD_DR3;
+		break;
+	case 57600:
+		clk_div = WC_BAUD_DR2;
+		break;
+	case 115200:
+		clk_div = WC_BAUD_DR1;
+		break;
+	case 230400:
+		if (max->clock & MAX3110_HIGH_CLK)
+			break;
+	default:
+		/* Pick the previous baud rate */
+		baud = max->baud;
+		clk_div = max->cur_conf & WC_BAUD_DIV_MASK;
+		tty_termios_encode_baud_rate(termios, baud, baud);
+	}
+
+	if (max->clock & MAX3110_HIGH_CLK) {
+		clk_div += 1;
+		/* High clk version max3110 doesn't support B300 */
+		if (baud == 300)
+			baud = 600;
+		if (baud == 230400)
+			clk_div = WC_BAUD_DR1;
+		tty_termios_encode_baud_rate(termios, baud, baud);
+	}
+
+	new_conf = (new_conf & ~WC_BAUD_DIV_MASK) | clk_div;
+
+	if (unlikely(termios->c_cflag & CMSPAR))
+		termios->c_cflag &= ~CMSPAR;
+
+	if (termios->c_cflag & CSTOPB)
+		new_conf |= WC_2_STOPBITS;
+	else
+		new_conf &= ~WC_2_STOPBITS;
+
+	if (termios->c_cflag & PARENB) {
+		new_conf |= WC_PARITY_ENABLE;
+		parity |= UART_LCR_PARITY;
+	} else
+		new_conf &= ~WC_PARITY_ENABLE;
+
+	if (!(termios->c_cflag & PARODD))
+		parity |= UART_LCR_EPAR;
+	max->parity = parity;
+
+	uart_update_timeout(port, termios->c_cflag, baud);
+
+	new_conf |= WC_TAG;
+	if (new_conf != max->cur_conf) {
+		if (!max3110_out(max, new_conf)) {
+			max->cur_conf = new_conf;
+			max->baud = baud;
+		}
+	}
+}
+
+/* Don't handle hw handshaking */
+static unsigned int serial_m3110_get_mctrl(struct uart_port *port)
+{
+	return TIOCM_DSR | TIOCM_CAR | TIOCM_DSR;
+}
+
+static void serial_m3110_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+}
+
+static void serial_m3110_break_ctl(struct uart_port *port, int break_state)
+{
+}
+
+static void serial_m3110_pm(struct uart_port *port, unsigned int state,
+			unsigned int oldstate)
+{
+}
+
+static void serial_m3110_enable_ms(struct uart_port *port)
+{
+}
+
+struct uart_ops serial_m3110_ops = {
+	.tx_empty	= serial_m3110_tx_empty,
+	.set_mctrl	= serial_m3110_set_mctrl,
+	.get_mctrl	= serial_m3110_get_mctrl,
+	.stop_tx	= serial_m3110_stop_tx,
+	.start_tx	= serial_m3110_start_tx,
+	.stop_rx	= serial_m3110_stop_rx,
+	.enable_ms	= serial_m3110_enable_ms,
+	.break_ctl	= serial_m3110_break_ctl,
+	.startup	= serial_m3110_startup,
+	.shutdown	= serial_m3110_shutdown,
+	.set_termios	= serial_m3110_set_termios,
+	.pm		= serial_m3110_pm,
+	.type		= serial_m3110_type,
+	.release_port	= serial_m3110_release_port,
+	.request_port	= serial_m3110_request_port,
+	.config_port	= serial_m3110_config_port,
+	.verify_port	= serial_m3110_verify_port,
+};
+
+static struct uart_driver serial_m3110_reg = {
+	.owner		= THIS_MODULE,
+	.driver_name	= "Maxim 3110",
+	.dev_name	= "ttyS",
+	.major		= TTY_MAJOR,
+	.minor		= 64,
+	.nr		= 1,
+	.cons		= &serial_m3110_console,
+};
+
+#ifdef CONFIG_PM
+static int serial_m3110_suspend(struct spi_device *spi, pm_message_t state)
+{
+	disable_irq(pmax->irq);
+	uart_suspend_port(&serial_m3110_reg, &pmax->port);
+	max3110_out(pmax, pmax->cur_conf | WC_SW_SHDI);
+	return 0;
+}
+
+static int serial_m3110_resume(struct spi_device *spi)
+{
+	max3110_out(pmax, pmax->cur_conf);
+	uart_resume_port(&serial_m3110_reg, &pmax->port);
+	enable_irq(pmax->irq);
+	return 0;
+}
+#else
+#define serial_m3110_suspend	NULL
+#define serial_m3110_resume	NULL
+#endif
+
+static int __devinit serial_m3110_probe(struct spi_device *spi)
+{
+	struct uart_max3110 *max;
+	int ret;
+	void *buffer;
+
+	max = kzalloc(sizeof(*max), GFP_KERNEL);
+	if (!max)
+		return -ENOMEM;
+
+	spi->bits_per_word = 16;
+	spi_setup(spi);
+
+	max->clock = MAX3110_HIGH_CLK;
+	max->port.type = PORT_MAX3110;
+	max->port.fifosize = 2;		/* Only have 16b buffer */
+	max->port.ops = &serial_m3110_ops;
+	max->port.line = 0;
+	max->port.dev = &spi->dev;
+	max->port.uartclk = 115200;
+
+	max->spi = spi;
+	max->name = spi->modalias;	/* Use spi name as the name */
+	max->irq = (u16)spi->irq;
+	spin_lock_init(&max->lock);
+
+	max->word_7bits = 0;
+	max->parity = 0;
+	max->baud = 0;
+
+	max->cur_conf = 0;
+	max->flags = 0;
+
+	buffer = (void *)__get_free_page(GFP_KERNEL);
+	if (!buffer) {
+		ret = -ENOMEM;
+		goto err_get_page;
+	}
+	max->con_xmit.buf = buffer;
+	max->con_xmit.head = max->con_xmit.tail = 0;
+
+	max->main_thread = kthread_run(max3110_main_thread,
+					max, "max3110_main");
+	if (IS_ERR(max->main_thread)) {
+		ret = PTR_ERR(max->main_thread);
+		goto err_kthread;
+	}
+
+	pmax = max;
+	/* Give membase a psudo value to pass serial_core's check */
+	max->port.membase = buffer;
+	uart_add_one_port(&serial_m3110_reg, &max->port);
+
+	return 0;
+
+err_kthread:
+	free_page((unsigned long)buffer);
+err_get_page:
+	pmax = NULL;
+	kfree(max);
+	return ret;
+}
+
+static int __devexit max3110_remove(struct spi_device *dev)
+{
+	struct uart_max3110 *max = pmax;
+
+	if (!pmax)
+		return 0;
+
+	pmax = NULL;
+	uart_remove_one_port(&serial_m3110_reg, &max->port);
+
+	free_page((unsigned long)max->con_xmit.buf);
+
+	if (max->main_thread)
+		kthread_stop(max->main_thread);
+
+	kfree(max);
+	return 0;
+}
+
+static struct spi_driver uart_max3110_driver = {
+	.driver = {
+			.name	= "spi_max3110",
+			.bus	= &spi_bus_type,
+			.owner	= THIS_MODULE,
+	},
+	.probe		= serial_m3110_probe,
+	.remove		= __devexit_p(max3110_remove),
+	.suspend	= serial_m3110_suspend,
+	.resume		= serial_m3110_resume,
+};
+
+static int __init serial_m3110_init(void)
+{
+	int ret = 0;
+
+	ret = uart_register_driver(&serial_m3110_reg);
+	if (ret)
+		return ret;
+
+	ret = spi_register_driver(&uart_max3110_driver);
+	if (ret)
+		uart_unregister_driver(&serial_m3110_reg);
+
+	return ret;
+}
+
+static void __exit serial_m3110_exit(void)
+{
+	spi_unregister_driver(&uart_max3110_driver);
+	uart_unregister_driver(&serial_m3110_reg);
+}
+
+module_init(serial_m3110_init);
+module_exit(serial_m3110_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("max3110-uart");
+MODULE_AUTHOR("Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
diff --git a/drivers/serial/max3110.h b/drivers/serial/max3110.h
new file mode 100644
index 0000000..4d58641
--- /dev/null
+++ b/drivers/serial/max3110.h
@@ -0,0 +1,61 @@
+#ifndef _MAX3110_HEAD_FILE_
+#define _MAX3110_HEAD_FILE_
+
+#define MAX3110_HIGH_CLK	0x1	/* 3.6864 MHZ */
+#define MAX3110_LOW_CLK		0x0	/* 1.8432 MHZ */
+
+/* Status bits for all 4 MAX3110 operate modes */
+#define MAX3110_READ_DATA_AVAILABLE	(1 << 15)
+#define MAX3110_WRITE_BUF_EMPTY		(1 << 14)
+
+#define WC_TAG			(3 << 14)
+#define RC_TAG			(1 << 14)
+#define WD_TAG			(2 << 14)
+#define RD_TAG			(0 << 14)
+
+/* Bits def for write configuration */
+#define WC_FIFO_ENABLE_MASK	(1 << 13)
+#define WC_FIFO_ENABLE		(0 << 13)
+
+#define WC_SW_SHDI		(1 << 12)
+
+#define WC_IRQ_MASK		(0xF << 8)
+#define WC_TXE_IRQ_ENABLE	(1 << 11)	/* TX empty irq */
+#define WC_RXA_IRQ_ENABLE	(1 << 10)	/* RX availabe irq */
+#define WC_PAR_HIGH_IRQ_ENABLE	(1 << 9)
+#define WC_REC_ACT_IRQ_ENABLE	(1 << 8)
+
+#define WC_IRDA_ENABLE		(1 << 7)
+
+#define WC_STOPBITS_MASK	(1 << 6)
+#define WC_2_STOPBITS		(1 << 6)
+#define WC_1_STOPBITS		(0 << 6)
+
+#define WC_PARITY_ENABLE_MASK	(1 << 5)
+#define WC_PARITY_ENABLE	(1 << 5)
+
+#define WC_WORDLEN_MASK		(1 << 4)
+#define WC_7BIT_WORD		(1 << 4)
+#define WC_8BIT_WORD		(0 << 4)
+
+#define WC_BAUD_DIV_MASK	(0xF)
+#define WC_BAUD_DR1		(0x0)
+#define WC_BAUD_DR2		(0x1)
+#define WC_BAUD_DR4		(0x2)
+#define WC_BAUD_DR8		(0x3)
+#define WC_BAUD_DR16		(0x4)
+#define WC_BAUD_DR32		(0x5)
+#define WC_BAUD_DR64		(0x6)
+#define WC_BAUD_DR128		(0x7)
+#define WC_BAUD_DR3		(0x8)
+#define WC_BAUD_DR6		(0x9)
+#define WC_BAUD_DR12		(0xA)
+#define WC_BAUD_DR24		(0xB)
+#define WC_BAUD_DR48		(0xC)
+#define WC_BAUD_DR96		(0xD)
+#define WC_BAUD_DR192		(0xE)
+#define WC_BAUD_DR384		(0xF)
+
+/* Maxim 3110 has 8 words RX FIFO and 1 word TX FIFO */
+#define M3110_RX_FIFO_DEPTH	8
+#endif
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 8c3dd36..119ba73 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
 
+/* Maxim M3110 */
+#define PORT_MAX3110	91
+
 #ifdef __KERNEL__
 
 #include <linux/compiler.h>
-- 
1.6.3.3

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev