[PATCH 2.6.16-git] [SERIAL] Adds DCC(JTAG) serial and the console emulation support

[PATCH 2.6.16-git] [SERIAL] Adds DCC(JTAG) serial and the console emulation support

[Hyok S. Choi]

From: Hyok S. Choi <hyok.choi@samsung.com>

Many of JTAG debuggers for ARM support DCC protocol over JTAG
connection, which is very useful to debug hardwares which has no
serial port. This patch adds DCC serial emulation and the console
support. System timer based polling method is used for the
emulation of serial input interrupt handling.

Signed-off-by: Hyok S. Choi <hyok.choi@samsung.com>
---

 drivers/serial/Kconfig      |   22 ++
 drivers/serial/Makefile     |    1 
 drivers/serial/dcc.c        |  550 
+++++++++++++++++++++++++++++++++++++++++++
 include/linux/serial_core.h |    3 
 4 files changed, 576 insertions(+), 0 deletions(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index fe0d8b8..4a7f059 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -325,6 +325,28 @@ config SERIAL_CLPS711X_CONSOLE
 	  your boot loader (lilo or loadlin) about how to pass options to the
 	  kernel at boot time.)
 
+config SERIAL_DCC
+	bool "JTAG ICE/ICD DCC serial port emulation support"
+	depends on ARM
+	select SERIAL_CORE
+	help
+	  This selects serial port emulation driver for ICE/ICD JTAG debugger
+	  (e.g. Trace32) for ARM architecture. You should make an terminal with
+	  DCC(JTAG1) protocol.
+
+	  if unsure, say N.
+ 
+config SERIAL_DCC_CONSOLE
+	bool "Support for console on JTAG ICE/ICD DCC"
+	depends on SERIAL_DCC
+	select SERIAL_CORE_CONSOLE
+	help
+	  Say Y here if you wish to use ICE/ICD JTAG DCC serial port emulation
+	  as the system console. you can use command line option "console=ttyJ0"
+	  for manual console driver setup.
+
+	  if unsure, say N.
+
 config SERIAL_S3C2410
 	tristate "Samsung S3C2410 Serial port support"
 	depends on ARM && ARCH_S3C2410
diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index d2b4c21..e408ff3 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_SERIAL_8250_AU1X00) += 8250
 obj-$(CONFIG_SERIAL_AMBA_PL010) += amba-pl010.o
 obj-$(CONFIG_SERIAL_AMBA_PL011) += amba-pl011.o
 obj-$(CONFIG_SERIAL_CLPS711X) += clps711x.o
+obj-$(CONFIG_SERIAL_DCC) += dcc.o
 obj-$(CONFIG_SERIAL_PXA) += pxa.o
 obj-$(CONFIG_SERIAL_SA1100) += sa1100.o
 obj-$(CONFIG_SERIAL_S3C2410) += s3c2410.o
diff --git a/drivers/serial/dcc.c b/drivers/serial/dcc.c
new file mode 100644
index 0000000..d73ccf6
--- /dev/null
+++ b/drivers/serial/dcc.c
@@ -0,0 +1,550 @@
+/*
+ *  linux/drivers/serial/dcc.c
+ *
+ *  serial port emulation driver for the JTAG DCC Terminal.
+ *
+ * DCC(JTAG1) protocol version for JTAG ICE/ICD Debuggers:
+ * 	Copyright (C) 2003, 2004, 2005 Hyok S. Choi (hyok.choi@samsung.com)
+ * 	SAMSUNG ELECTRONICS Co.,Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ *  Changelog:
+ *   Oct-2003 Hyok S. Choi	Created
+ *   Feb-2004 Hyok S. Choi 	Updated for serial_core.c and 2.6 kernel
+ *   Apr-2004 Hyok S. Choi 	xmit_string_CR added
+ *   Mar-2005 Hyok S. Choi	renamed from T32 to DCC
+ *
+ */
+
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/tty.h>
+#include <linux/ioport.h>
+#include <linux/init.h>
+#include <linux/serial.h>
+#include <linux/console.h>
+#include <linux/sysrq.h>
+#include <linux/tty_flip.h>
+#include <linux/major.h>
+
+#include <asm/io.h>
+#include <asm/irq.h>
+
+#include <linux/serial_core.h>
+
+/*
+ * if real irq interrupt used for receiving character,
+ * uncomment below line. Unless polling method id used.
+ */
+//#define DCC_IRQ_USED
+
+#ifndef  DCC_IRQ_USED
+static struct work_struct dcc_poll_task;
+static void dcc_poll(void *data);
+#endif
+
+#define UART_NR			1	/* we have only one JTAG port */
+
+#define SERIAL_DCC_NAME		"ttyJ"
+#define SERIAL_DCC_MAJOR	4
+#define SERIAL_DCC_MINOR	64
+
+static int __inline__ __check_JTAG_RX_FLAG(void)
+{
+	int __ret=0;
+	__asm__ __volatile__(
+		"	mrc		p14, 0, %0, c0, c0 	@ read comms control reg\n"
+		"	and		%0, %0, #1		@ jtag read buffer status"
+		: "=r" (__ret)
+		);
+
+	/* if   __ret 	== 0 : no input yet
+				== 1 : a character pending */
+	return __ret;	
+}
+
+static void __inline__ __get_JTAG_RX(volatile char *p)
+{
+	__asm__ __volatile__(
+		"	mrc		p14, 0, r3, c1, c0 	@ read comms data reg to r5\n"
+		"	strb 		r3, 	[%0]		@ str a char"
+		: /* no output */
+		: "r" (p)
+		: "r3",  "memory");
+}
+
+
+static int __inline__ __check_JTAG_TX_FLAG(void)
+{
+	int __ret=0;
+	__asm__ __volatile__(
+		"	mrc		p14, 0, %0, c0, c0 	@ read comms control reg\n"
+		"	and 		%0, %0, #2		@ the read buffer status"
+		: "=r" (__ret)
+		);
+
+	/* if   __ret 	== 0 : tx is available
+				== 2 : tx busy */
+	return __ret;	
+}
+
+void xmit_string(char *p, int len)
+{
+#ifndef CONFIG_JTAG_DCC_OUTPUT_DISABLE
+	/*
+		r0 = string	; string address
+		r1 = 2		; state check bit (write)
+		r4 = *string	; character
+		r7 = 0		; count
+	*/
+	__asm__ __volatile__(
+		"	mov r7, #0\n"
+		"1: 	mrc	p14, 0, r3, c0, c0 	@ read comms control reg\n"
+		"	and r3, r3, #2			@ the write buffer status\n"
+		"	cmp r3, #2			@ is it available?\n"
+		"	beq 1b				@ is not, wait till then\n"
+		"	ldrb r4, [%0]			@ load a char\n"
+		"	mcr p14, 0, r4, c1, c0		@ write it\n"
+		"	add %0, %0, #1			@ str address increase one\n"
+		"	add r7, r7, #1			@ count increase\n"
+		"	cmp r7, %1			@ compare with str length\n"
+		"	bne 1b				@ if it is not yet, loop"
+		: /* no output register */
+		: "r" (p), "r" (len)
+		: "r7", "r3", "r4");
+#endif
+}
+
+void xmit_string_CR(char *p, int len)
+{
+#ifndef CONFIG_JTAG_DCC_OUTPUT_DISABLE
+	/*
+		r0 = string	; string address
+		r1 = 2		; state check bit (write)
+		r4 = *string	; character
+		r7 = 0		; count
+	*/
+	__asm__ __volatile__(
+		"	mov r7, #0\n"
+		"	ldrb r4, [%0]			@ load a char\n"
+		"1: 	mrc	p14, 0, r3, c0, c0 	@ read comms control reg\n"
+		"	and r3, r3, #2			@ the write buffer status\n"
+		"	cmp r3, #2			@ is it available?\n"
+		"	beq 1b				@ is not, wait till then\n"
+		"	mcr p14, 0, r4, c1, c0		@ write it\n"
+		"	cmp r4, #0x0a			@ is it LF?\n"
+		"	bne 2f				@ if it is not, continue\n"
+		"	mov r4, #0x0d			@ set the CR\n"
+		"	b   1b				@ loop for writing CR\n"			
+		"2:	ldrb r4, [%0, #1]!		@ load a char\n"
+		"	add r7, r7, #1			@ count increase\n"
+		"	cmp r7, %1			@ compare with str length\n"
+		"	bne 1b				@ if it is not yet, loop"
+		: /* no output register */
+		: "r" (p), "r" (len)
+		: "r7", "r3", "r4");
+#endif
+}
+
+
+static void
+dcc_stop_tx(struct uart_port *port)
+{
+}
+
+static inline void
+dcc_transmit_buffer(struct uart_port *port)
+{
+	struct circ_buf *xmit = &port->info->xmit;
+
+	int pendings = uart_circ_chars_pending(xmit);
+
+	if(pendings + xmit->tail > UART_XMIT_SIZE)
+	{
+		xmit_string(&(xmit->buf[xmit->tail]), UART_XMIT_SIZE - xmit->tail);
+		xmit_string(&(xmit->buf[0]), xmit->head);
+	} else
+		xmit_string(&(xmit->buf[xmit->tail]), pendings);
+	
+	xmit->tail = (xmit->tail + pendings) & (UART_XMIT_SIZE-1);
+        port->icount.tx += pendings;
+
+	if (uart_circ_empty(xmit))
+		dcc_stop_tx(port); 
+}
+
+static inline void
+dcc_transmit_x_char(struct uart_port *port)
+{
+	xmit_string(&port->x_char, 1);
+	port->icount.tx++;
+	port->x_char = 0;
+}
+
+static void
+dcc_start_tx(struct uart_port *port)
+{
+    dcc_transmit_buffer(port);
+}
+
+static void
+dcc_stop_rx(struct uart_port *port)
+{
+}
+
+static void
+dcc_enable_ms(struct uart_port *port)
+{
+}
+
+static inline void
+dcc_rx_chars(struct uart_port *port)
+{
+	unsigned char ch;
+	struct tty_struct *tty = port->info->tty;
+
+	/*
+	 * check input.
+	 * checking JTAG flag is better to resolve the status test.
+	 * incount is NOT used for JTAG1 protocol.
+	 */
+
+	if (__check_JTAG_RX_FLAG())
+		/* if   __ret 	== 0 : no input yet
+					== 1 : a character pending */
+	{
+		/* for JTAG 1 protocol, incount is always 1. */
+		__get_JTAG_RX(&ch);
+
+		if (tty) {
+			tty_insert_flip_char(tty, ch, TTY_NORMAL);
+			port->icount.rx++;
+			tty_flip_buffer_push(tty);
+		}
+	}
+}
+
+static inline void
+dcc_overrun_chars(struct uart_port *port)
+{
+	port->icount.overrun++;
+}
+
+static inline void
+dcc_tx_chars(struct uart_port *port)
+{
+	struct circ_buf *xmit = &port->info->xmit;
+
+	if (port->x_char) {
+		dcc_transmit_x_char(port);
+		return; 
+	}
+
+	if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
+		dcc_stop_tx(port);
+		return;
+	}
+
+	dcc_transmit_buffer(port);
+
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+		uart_write_wakeup(port);
+}
+
+#ifdef DCC_IRQ_USED /* real IRQ used */
+static irqreturn_t
+dcc_int(int irq, void *dev_id, struct pt_regs *regs)
+{
+	struct uart_port *port = dev_id;
+	int handled = 0;
+
+	spin_lock(&port->lock);
+	
+	dcc_rx_chars(port);
+	dcc_tx_chars(port);
+
+	handled = 1;
+	spin_unlock(&port->lock);
+	
+	return IRQ_RETVAL(handled);
+}
+
+#else /* emulation by scheduled work */
+static void
+dcc_poll(void *data)
+{
+	struct uart_port *port = data;
+
+	spin_lock(&port->lock);
+	
+	dcc_rx_chars(port);
+	dcc_tx_chars(port);
+
+	schedule_delayed_work(&dcc_poll_task, 1);
+
+	spin_unlock(&port->lock);
+	
+}
+#endif /* end of DCC_IRQ_USED */
+
+static unsigned int
+dcc_tx_empty(struct uart_port *port)
+{
+	return TIOCSER_TEMT;
+}
+
+static unsigned int
+dcc_get_mctrl(struct uart_port *port)
+{
+	return 0;
+}
+
+static void
+dcc_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+}
+
+static void
+dcc_break_ctl(struct uart_port *port, int break_state)
+{
+}
+
+static int dcc_startup(struct uart_port *port)
+{
+#ifdef DCC_IRQ_USED /* real IRQ used */
+	int retval;
+
+	/* Allocate the IRQ */
+	retval = request_irq(port->irq, dcc_int, SA_INTERRUPT,
+			     "serial_dcc", port);
+	if (retval)
+		return retval;
+#else /* emulation */
+	/* Initialize the work, and shcedule it. */
+	INIT_WORK(&dcc_poll_task, dcc_poll, port);
+	schedule_delayed_work(&dcc_poll_task, 1);
+#endif
+
+	return 0;
+}
+
+static void dcc_shutdown(struct uart_port *port)
+{
+}
+
+static void
+dcc_set_termios(struct uart_port *port, struct termios *termios,
+		   struct termios *old)
+{
+#ifdef DCC_IRQ_USED
+	unsigned long flags;
+#endif
+	unsigned int baud, quot;
+
+	/*
+	 * We don't support parity, stop bits, or anything other
+	 * than 8 bits, so clear these termios flags.
+	 */
+	termios->c_cflag &= ~(CSIZE | CSTOPB | PARENB | PARODD | CREAD);
+	termios->c_cflag |= CS8;
+
+	/*
+	 * We don't appear to support any error conditions either.
+	 */
+	termios->c_iflag &= ~(INPCK | IGNPAR | IGNBRK | BRKINT);
+
+	/*
+	 * Ask the core to calculate the divisor for us.
+	 */
+	baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/16); 
+	quot = uart_get_divisor(port, baud);
+
+#ifdef DCC_IRQ_USED
+	spin_lock_irqsave(&port->lock, flags);
+#endif
+
+	uart_update_timeout(port, termios->c_cflag, baud);
+
+#ifdef DCC_IRQ_USED
+	spin_unlock_irqrestore(&port->lock, flags);
+#endif
+}
+
+static const char *dcc_type(struct uart_port *port)
+{
+	return port->type == PORT_DCC_JTAG1 ? "DCC" : NULL;
+}
+
+static void dcc_release_port(struct uart_port *port)
+{
+}
+
+static int dcc_request_port(struct uart_port *port)
+{
+	return 0;
+}
+
+/*
+ * Configure/autoconfigure the port.
+ */
+static void dcc_config_port(struct uart_port *port, int flags)
+{
+        if (flags & UART_CONFIG_TYPE) {
+                port->type = PORT_DCC_JTAG1;
+                dcc_request_port(port);
+        }
+}
+        
+/*      
+ * verify the new serial_struct (for TIOCSSERIAL).
+ */     
+static int dcc_verify_port(struct uart_port *port, struct serial_struct *ser)
+{
+        int ret = 0;
+        if (ser->type != PORT_UNKNOWN && ser->type != PORT_DCC_JTAG1)
+                ret = -EINVAL;
+        if (ser->irq < 0 || ser->irq >= NR_IRQS)
+                ret = -EINVAL;
+        if (ser->baud_base < 9600)
+                ret = -EINVAL;
+	return ret;
+}
+
+static struct uart_ops dcc_pops = {
+	.tx_empty	= dcc_tx_empty,
+	.set_mctrl	= dcc_set_mctrl,
+	.get_mctrl	= dcc_get_mctrl,
+	.stop_tx	= dcc_stop_tx,
+	.start_tx	= dcc_start_tx,
+	.stop_rx	= dcc_stop_rx,
+	.enable_ms	= dcc_enable_ms,
+	.break_ctl	= dcc_break_ctl,
+	.startup	= dcc_startup,
+	.shutdown	= dcc_shutdown,
+	.set_termios	= dcc_set_termios,
+	.type		= dcc_type,
+	.release_port	= dcc_release_port,
+	.request_port	= dcc_request_port,
+	.config_port	= dcc_config_port,
+	.verify_port	= dcc_verify_port,
+};
+
+static struct uart_port dcc_ports[UART_NR] = {
+	{
+		.membase	= (char*)0x12345678,	/* we need these garbages */
+		.mapbase	= 0x12345678,		/* for serial_core.c */
+		.iotype		= SERIAL_IO_MEM,	
+#ifdef DCC_IRQ_USED
+		.irq		= INT_N_EXT0,
+#else
+		.irq		= 0,
+#endif
+		.uartclk	= 14745600,			 
+		.fifosize	= 0,
+		.ops		= &dcc_pops,
+		.flags		= ASYNC_BOOT_AUTOCONF,
+		.line		= 0,
+	},
+};
+
+
+#ifdef CONFIG_SERIAL_DCC_CONSOLE
+
+static void
+dcc_console_write(struct console *co, const char *s, unsigned int count)
+{
+	xmit_string_CR((char*)s, count);
+}
+
+/*
+ * Read the current UART setup.
+ */
+static void __init
+dcc_console_get_options(struct uart_port *port, int *baud, int *parity, int 
*bits)
+{
+	*baud = 9600;
+	*parity = 'n';
+	*bits = 8;
+}
+
+static int __init
+dcc_console_setup(struct console *co, char *options)
+{
+	struct uart_port *port;
+	int baud = 9600;
+	int bits = 8;
+	int parity = 'n';
+	int flow = 'n';
+
+	if (co->index >= UART_NR)
+		co->index = 0;
+	port = &dcc_ports[co->index];
+
+	if (options)
+		uart_parse_options(options, &baud, &parity, &bits, &flow);
+	else
+		dcc_console_get_options(port, &baud, &parity, &bits);
+
+	return uart_set_options(port, co, baud, parity, bits, flow);
+}
+
+extern struct uart_driver dcc_reg;
+static struct console dcc_console = {
+	.name		= SERIAL_DCC_NAME,
+	.write		= dcc_console_write,
+	.device		= uart_console_device,
+	.setup		= dcc_console_setup,
+	.flags		= CON_PRINTBUFFER,
+	.index		= -1,
+	.data		= &dcc_reg,
+};
+
+static int __init dcc_console_init(void)
+{
+	register_console(&dcc_console);
+	return 0;
+}
+console_initcall(dcc_console_init);
+
+#define DCC_CONSOLE		&dcc_console
+#else
+#define DCC_CONSOLE		NULL
+#endif
+
+static struct uart_driver dcc_reg = {
+	.owner			= THIS_MODULE,
+	.driver_name		= SERIAL_DCC_NAME,
+	.dev_name		= SERIAL_DCC_NAME,
+	.major			= SERIAL_DCC_MAJOR,
+	.minor			= SERIAL_DCC_MINOR,
+	.nr			= UART_NR,
+	.cons			= DCC_CONSOLE,
+};
+
+static int __init
+dcc_init(void)
+{
+	int i, ret = 0;
+
+	printk(KERN_INFO "DCC: JTAG1 Serial emulation driver driver $Revision: 1.1 
$\n");
+
+	ret = uart_register_driver(&dcc_reg);
+
+	if (ret)
+		return ret;
+
+	for (i = 0; i < UART_NR; i++)
+		uart_add_one_port(&dcc_reg, &dcc_ports[i]);
+
+	return ret;
+}
+
+__initcall(dcc_init);
+
+MODULE_DESCRIPTION("DCC(JTAG1) JTAG debugger console emulation driver");
+MODULE_AUTHOR("Hyok S. Choi <hyok.choi@samsung.com>");
+MODULE_SUPPORTED_DEVICE("ttyJ");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index c32e60e..561bc6a 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -130,6 +130,9 @@
 /* SUN4V Hypervisor Console */
 #define PORT_SUNHV	72
 
+/* DCC(JTAG) emulation port types */
+#define PORT_DCC_JTAG1	73
+
 #ifdef __KERNEL__
 
 #include <linux/config.h>


--
Hyok
ARM Linux 2.6 MPU/noMMU Project http://opensrc.sec.samsung.com/

Re: [PATCH 2.6.16-git] [SERIAL] Adds DCC(JTAG) serial and the console emulation support

[Russell King]

On Wed, Mar 29, 2006 at 04:11:25PM +0900, Hyok S. Choi wrote:
> diff --git a/drivers/serial/dcc.c b/drivers/serial/dcc.c

Some comments on this driver below.

> new file mode 100644
> index 0000000..d73ccf6
> --- /dev/null
> +++ b/drivers/serial/dcc.c
> @@ -0,0 +1,550 @@
> +/*
> + *  linux/drivers/serial/dcc.c
> + *
> + *  serial port emulation driver for the JTAG DCC Terminal.
> + *
> + * DCC(JTAG1) protocol version for JTAG ICE/ICD Debuggers:
> + * 	Copyright (C) 2003, 2004, 2005 Hyok S. Choi (hyok.choi@samsung.com)
> + * 	SAMSUNG ELECTRONICS Co.,Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + *  Changelog:
> + *   Oct-2003 Hyok S. Choi	Created
> + *   Feb-2004 Hyok S. Choi 	Updated for serial_core.c and 2.6 kernel
> + *   Apr-2004 Hyok S. Choi 	xmit_string_CR added
> + *   Mar-2005 Hyok S. Choi	renamed from T32 to DCC
> + *
> + */
> +
> +#include <linux/config.h>
> +#include <linux/module.h>
> +#include <linux/tty.h>
> +#include <linux/ioport.h>
> +#include <linux/init.h>
> +#include <linux/serial.h>
> +#include <linux/console.h>
> +#include <linux/sysrq.h>
> +#include <linux/tty_flip.h>
> +#include <linux/major.h>
> +
> +#include <asm/io.h>
> +#include <asm/irq.h>
> +
> +#include <linux/serial_core.h>
> +
> +/*
> + * if real irq interrupt used for receiving character,
> + * uncomment below line. Unless polling method id used.
> + */
> +//#define DCC_IRQ_USED
> +
> +#ifndef  DCC_IRQ_USED
> +static struct work_struct dcc_poll_task;
> +static void dcc_poll(void *data);
> +#endif
> +
> +#define UART_NR			1	/* we have only one JTAG port */
> +
> +#define SERIAL_DCC_NAME		"ttyJ"
> +#define SERIAL_DCC_MAJOR	4
> +#define SERIAL_DCC_MINOR	64

Is it really a good idea to re-use the same major and minor numbers that
are used for 8250?  What if you want to use DCC with a board that has
8250-based serial ports on as well?

> +
> +static int __inline__ __check_JTAG_RX_FLAG(void)

"static inline int" please.

> +{
> +	int __ret=0;
> +	__asm__ __volatile__(
> +		"	mrc		p14, 0, %0, c0, c0 	@ read comms control reg\n"
> +		"	and		%0, %0, #1		@ jtag read buffer status"
> +		: "=r" (__ret)
> +		);
> +
> +	/* if   __ret 	== 0 : no input yet
> +				== 1 : a character pending */

Wouldn't it be clearer to have:

static inline u32 read_dcc_status(void)
{
	u32 status;

	asm("mrc p14, 0, %0, c0, c0" : "=r" (status));

	return status;
}

and if you want to check the RX flag in the rest of the code:

	if (read_dcc_status() & DCC_STAT_RX) {
		/* do pending character stuff */
	}

?

> +	return __ret;	
> +}
> +
> +static void __inline__ __get_JTAG_RX(volatile char *p)
> +{
> +	__asm__ __volatile__(
> +		"	mrc		p14, 0, r3, c1, c0 	@ read comms data reg to r5\n"
> +		"	strb 		r3, 	[%0]		@ str a char"
> +		: /* no output */
> +		: "r" (p)
> +		: "r3",  "memory");
> +}

Ditto comments above.  The only use of this I can find is:

		__get_JTAG_RX(&ch);

and it's crying out to be:

static inline char dcc_getchar(void)
{
	char c;

	asm("mrc p14, 0, %0, c1, c0" : "=r" (c));

	return c;
}

...

		ch = dcc_getchar();

Also, a write_dcc_char(char c) would be useful.

> +static int __inline__ __check_JTAG_TX_FLAG(void)
> +{
> +	int __ret=0;
> +	__asm__ __volatile__(
> +		"	mrc		p14, 0, %0, c0, c0 	@ read comms control reg\n"
> +		"	and 		%0, %0, #2		@ the read buffer status"
> +		: "=r" (__ret)
> +		);
> +
> +	/* if   __ret 	== 0 : tx is available
> +				== 2 : tx busy */
> +	return __ret;	
> +}

Same comments as for __check_JTAG_RX_FLAG().

> +
> +void xmit_string(char *p, int len)
> +{
> +#ifndef CONFIG_JTAG_DCC_OUTPUT_DISABLE
> +	/*
> +		r0 = string	; string address
> +		r1 = 2		; state check bit (write)
> +		r4 = *string	; character
> +		r7 = 0		; count
> +	*/
> +	__asm__ __volatile__(
> +		"	mov r7, #0\n"
> +		"1: 	mrc	p14, 0, r3, c0, c0 	@ read comms control reg\n"
> +		"	and r3, r3, #2			@ the write buffer status\n"
> +		"	cmp r3, #2			@ is it available?\n"
> +		"	beq 1b				@ is not, wait till then\n"
> +		"	ldrb r4, [%0]			@ load a char\n"
> +		"	mcr p14, 0, r4, c1, c0		@ write it\n"
> +		"	add %0, %0, #1			@ str address increase one\n"
> +		"	add r7, r7, #1			@ count increase\n"
> +		"	cmp r7, %1			@ compare with str length\n"
> +		"	bne 1b				@ if it is not yet, loop"
> +		: /* no output register */
> +		: "r" (p), "r" (len)
> +		: "r7", "r3", "r4");
> +#endif
> +}

Does this really need to be written all in assembly?
Shouldn't it be declared static?

Wouldn't something like the following be better?

static void dcc_putchar(struct uart_port *port, char c)
{
	while (read_dcc_status() & DCC_STAT_TX_FULL)
		cpu_relax();
	write_dcc_char(c);
}

static void xmit_string(struct uart_port *port, const char *p, int len)
{
	for (; len; len--, p++)
		dcc_putchar(port, *p);
}

> +
> +void xmit_string_CR(char *p, int len)
> +{
> +#ifndef CONFIG_JTAG_DCC_OUTPUT_DISABLE
> +	/*
> +		r0 = string	; string address
> +		r1 = 2		; state check bit (write)
> +		r4 = *string	; character
> +		r7 = 0		; count
> +	*/
> +	__asm__ __volatile__(
> +		"	mov r7, #0\n"
> +		"	ldrb r4, [%0]			@ load a char\n"
> +		"1: 	mrc	p14, 0, r3, c0, c0 	@ read comms control reg\n"
> +		"	and r3, r3, #2			@ the write buffer status\n"
> +		"	cmp r3, #2			@ is it available?\n"
> +		"	beq 1b				@ is not, wait till then\n"
> +		"	mcr p14, 0, r4, c1, c0		@ write it\n"
> +		"	cmp r4, #0x0a			@ is it LF?\n"
> +		"	bne 2f				@ if it is not, continue\n"
> +		"	mov r4, #0x0d			@ set the CR\n"
> +		"	b   1b				@ loop for writing CR\n"			
> +		"2:	ldrb r4, [%0, #1]!		@ load a char\n"
> +		"	add r7, r7, #1			@ count increase\n"
> +		"	cmp r7, %1			@ compare with str length\n"
> +		"	bne 1b				@ if it is not yet, loop"
> +		: /* no output register */
> +		: "r" (p), "r" (len)
> +		: "r7", "r3", "r4");
> +#endif
> +}

No need - use uart_console_write() for this, which will do the LF -> CRLF
translation for you.  All you need to do is to supply a function to do
the individual character based writing.  You can pass dcc_putchar()
as the putchar function (which is why I wrote xmit_string that way
above.)

> +
> +
> +static void
> +dcc_stop_tx(struct uart_port *port)
> +{
> +}
> +
> +static inline void
> +dcc_transmit_buffer(struct uart_port *port)
> +{
> +	struct circ_buf *xmit = &port->info->xmit;
> +

Blank line not required.

> +	int pendings = uart_circ_chars_pending(xmit);
> +
> +	if(pendings + xmit->tail > UART_XMIT_SIZE)
> +	{

Interesting indentation style mix.

> +		xmit_string(&(xmit->buf[xmit->tail]), UART_XMIT_SIZE - xmit->tail);
> +		xmit_string(&(xmit->buf[0]), xmit->head);
> +	} else
> +		xmit_string(&(xmit->buf[xmit->tail]), pendings);
> +	
> +	xmit->tail = (xmit->tail + pendings) & (UART_XMIT_SIZE-1);
> +        port->icount.tx += pendings;

Tabs vs spaces indentation.  Please use a consistent indentation style.

> +
> +	if (uart_circ_empty(xmit))
> +		dcc_stop_tx(port); 

dcc_stop_tx itself is empty, so this doesn't serve a useful purpose.

> +}
> +
> +static inline void
> +dcc_transmit_x_char(struct uart_port *port)
> +{
> +	xmit_string(&port->x_char, 1);

Another potential user of dcc_putchar().

> +	port->icount.tx++;
> +	port->x_char = 0;
> +}
> +
> +static void
> +dcc_start_tx(struct uart_port *port)
> +{
> +    dcc_transmit_buffer(port);
> +}
> +
> +static void
> +dcc_stop_rx(struct uart_port *port)
> +{
> +}
> +
> +static void
> +dcc_enable_ms(struct uart_port *port)
> +{
> +}

Maybe call all the empty functions which take just a uart_port

static void dcc_dummy(struct uart_port *port)
{
}

and use it in the uart_ops structure as appropriate.

> +
> +static inline void
> +dcc_rx_chars(struct uart_port *port)
> +{
> +	unsigned char ch;
> +	struct tty_struct *tty = port->info->tty;
> +
> +	/*
> +	 * check input.
> +	 * checking JTAG flag is better to resolve the status test.
> +	 * incount is NOT used for JTAG1 protocol.
> +	 */
> +
> +	if (__check_JTAG_RX_FLAG())
> +		/* if   __ret 	== 0 : no input yet
> +					== 1 : a character pending */

	if (read_dcc_status() & DCC_STAT_RX) {

> +	{
> +		/* for JTAG 1 protocol, incount is always 1. */
> +		__get_JTAG_RX(&ch);

		ch = dcc_getchar();

> +
> +		if (tty) {
> +			tty_insert_flip_char(tty, ch, TTY_NORMAL);
> +			port->icount.rx++;
> +			tty_flip_buffer_push(tty);
> +		}
> +	}
> +}
> +
> +static inline void
> +dcc_overrun_chars(struct uart_port *port)
> +{
> +	port->icount.overrun++;
> +}
> +
> +static inline void
> +dcc_tx_chars(struct uart_port *port)
> +{
> +	struct circ_buf *xmit = &port->info->xmit;
> +
> +	if (port->x_char) {
> +		dcc_transmit_x_char(port);
> +		return; 
> +	}
> +
> +	if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
> +		dcc_stop_tx(port);
> +		return;
> +	}

If we're using interrupt mode, given that dcc_stop_tx() is empty, how
do we stop spinning on the transmit interrupt?

> +
> +	dcc_transmit_buffer(port);
> +
> +	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> +		uart_write_wakeup(port);
> +}
> +
> +#ifdef DCC_IRQ_USED /* real IRQ used */
> +static irqreturn_t
> +dcc_int(int irq, void *dev_id, struct pt_regs *regs)
> +{
> +	struct uart_port *port = dev_id;
> +	int handled = 0;
> +
> +	spin_lock(&port->lock);
> +	
> +	dcc_rx_chars(port);
> +	dcc_tx_chars(port);
> +
> +	handled = 1;
> +	spin_unlock(&port->lock);
> +	
> +	return IRQ_RETVAL(handled);
> +}
> +
> +#else /* emulation by scheduled work */
> +static void
> +dcc_poll(void *data)
> +{
> +	struct uart_port *port = data;
> +
> +	spin_lock(&port->lock);
> +	
> +	dcc_rx_chars(port);
> +	dcc_tx_chars(port);
> +
> +	schedule_delayed_work(&dcc_poll_task, 1);
> +
> +	spin_unlock(&port->lock);
> +	
> +}
> +#endif /* end of DCC_IRQ_USED */
> +
> +static unsigned int
> +dcc_tx_empty(struct uart_port *port)
> +{
> +	return TIOCSER_TEMT;
> +}
> +
> +static unsigned int
> +dcc_get_mctrl(struct uart_port *port)
> +{
> +	return 0;

Should return TIOCM_CTS | TIOCM_DSR | TIOCM_CD if control lines aren't
implemented.

> +}
> +
> +static void
> +dcc_set_mctrl(struct uart_port *port, unsigned int mctrl)
> +{
> +}
> +
> +static void
> +dcc_break_ctl(struct uart_port *port, int break_state)
> +{
> +}
> +
> +static int dcc_startup(struct uart_port *port)
> +{
> +#ifdef DCC_IRQ_USED /* real IRQ used */
> +	int retval;
> +
> +	/* Allocate the IRQ */
> +	retval = request_irq(port->irq, dcc_int, SA_INTERRUPT,
> +			     "serial_dcc", port);
> +	if (retval)
> +		return retval;
> +#else /* emulation */
> +	/* Initialize the work, and shcedule it. */
> +	INIT_WORK(&dcc_poll_task, dcc_poll, port);
> +	schedule_delayed_work(&dcc_poll_task, 1);
> +#endif
> +
> +	return 0;
> +}
> +
> +static void dcc_shutdown(struct uart_port *port)
> +{

Shouldn't the dcc_poll_task be stopped somehow here?

> +}
> +
> +static void
> +dcc_set_termios(struct uart_port *port, struct termios *termios,
> +		   struct termios *old)
> +{
> +#ifdef DCC_IRQ_USED
> +	unsigned long flags;
> +#endif
> +	unsigned int baud, quot;
> +
> +	/*
> +	 * We don't support parity, stop bits, or anything other
> +	 * than 8 bits, so clear these termios flags.
> +	 */
> +	termios->c_cflag &= ~(CSIZE | CSTOPB | PARENB | PARODD | CREAD);
> +	termios->c_cflag |= CS8;
> +
> +	/*
> +	 * We don't appear to support any error conditions either.
> +	 */
> +	termios->c_iflag &= ~(INPCK | IGNPAR | IGNBRK | BRKINT);
> +
> +	/*
> +	 * Ask the core to calculate the divisor for us.
> +	 */
> +	baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/16); 
> +	quot = uart_get_divisor(port, baud);
> +
> +#ifdef DCC_IRQ_USED
> +	spin_lock_irqsave(&port->lock, flags);
> +#endif
> +
> +	uart_update_timeout(port, termios->c_cflag, baud);
> +
> +#ifdef DCC_IRQ_USED
> +	spin_unlock_irqrestore(&port->lock, flags);
> +#endif
> +}
> +
> +static const char *dcc_type(struct uart_port *port)
> +{
> +	return port->type == PORT_DCC_JTAG1 ? "DCC" : NULL;
> +}
> +
> +static void dcc_release_port(struct uart_port *port)
> +{
> +}
> +
> +static int dcc_request_port(struct uart_port *port)
> +{
> +	return 0;
> +}
> +
> +/*
> + * Configure/autoconfigure the port.
> + */
> +static void dcc_config_port(struct uart_port *port, int flags)
> +{
> +        if (flags & UART_CONFIG_TYPE) {
> +                port->type = PORT_DCC_JTAG1;
> +                dcc_request_port(port);
> +        }
> +}
> +        
> +/*      
> + * verify the new serial_struct (for TIOCSSERIAL).
> + */     
> +static int dcc_verify_port(struct uart_port *port, struct serial_struct *ser)
> +{
> +        int ret = 0;
> +        if (ser->type != PORT_UNKNOWN && ser->type != PORT_DCC_JTAG1)
> +                ret = -EINVAL;
> +        if (ser->irq < 0 || ser->irq >= NR_IRQS)
> +                ret = -EINVAL;
> +        if (ser->baud_base < 9600)
> +                ret = -EINVAL;
> +	return ret;

Weirdo indentation style strikes again.

> +}
> +
> +static struct uart_ops dcc_pops = {
> +	.tx_empty	= dcc_tx_empty,
> +	.set_mctrl	= dcc_set_mctrl,
> +	.get_mctrl	= dcc_get_mctrl,
> +	.stop_tx	= dcc_stop_tx,
> +	.start_tx	= dcc_start_tx,
> +	.stop_rx	= dcc_stop_rx,
> +	.enable_ms	= dcc_enable_ms,
> +	.break_ctl	= dcc_break_ctl,
> +	.startup	= dcc_startup,
> +	.shutdown	= dcc_shutdown,
> +	.set_termios	= dcc_set_termios,
> +	.type		= dcc_type,
> +	.release_port	= dcc_release_port,
> +	.request_port	= dcc_request_port,
> +	.config_port	= dcc_config_port,
> +	.verify_port	= dcc_verify_port,
> +};
> +
> +static struct uart_port dcc_ports[UART_NR] = {

If there's only one port, this array isn't required.

> +	{
> +		.membase	= (char*)0x12345678,	/* we need these garbages */
> +		.mapbase	= 0x12345678,		/* for serial_core.c */
> +		.iotype		= SERIAL_IO_MEM,	

.iotype should be UPIO_xxx not SERIAL_IO_xxx.

> +#ifdef DCC_IRQ_USED
> +		.irq		= INT_N_EXT0,
> +#else
> +		.irq		= 0,
> +#endif
> +		.uartclk	= 14745600,			 
> +		.fifosize	= 0,
> +		.ops		= &dcc_pops,
> +		.flags		= ASYNC_BOOT_AUTOCONF,

.flags should be UPF_xxx not ASYNC_xxx.

> +		.line		= 0,
> +	},
> +};
> +
> +
> +#ifdef CONFIG_SERIAL_DCC_CONSOLE
> +
> +static void
> +dcc_console_write(struct console *co, const char *s, unsigned int count)
> +{
> +	xmit_string_CR((char*)s, count);
> +}
> +
> +/*
> + * Read the current UART setup.
> + */
> +static void __init
> +dcc_console_get_options(struct uart_port *port, int *baud, int *parity, int 
> *bits)
> +{
> +	*baud = 9600;
> +	*parity = 'n';
> +	*bits = 8;
> +}

Unnecessary function.

> +
> +static int __init
> +dcc_console_setup(struct console *co, char *options)
> +{
> +	struct uart_port *port;
> +	int baud = 9600;
> +	int bits = 8;
> +	int parity = 'n';
> +	int flow = 'n';
> +
> +	if (co->index >= UART_NR)
> +		co->index = 0;
> +	port = &dcc_ports[co->index];

If you're not likely to have more than one port, this doesn't make sense.

> +
> +	if (options)
> +		uart_parse_options(options, &baud, &parity, &bits, &flow);
> +	else
> +		dcc_console_get_options(port, &baud, &parity, &bits);
> +
> +	return uart_set_options(port, co, baud, parity, bits, flow);
> +}
> +
> +extern struct uart_driver dcc_reg;
> +static struct console dcc_console = {
> +	.name		= SERIAL_DCC_NAME,
> +	.write		= dcc_console_write,
> +	.device		= uart_console_device,
> +	.setup		= dcc_console_setup,
> +	.flags		= CON_PRINTBUFFER,
> +	.index		= -1,
> +	.data		= &dcc_reg,
> +};
> +
> +static int __init dcc_console_init(void)
> +{
> +	register_console(&dcc_console);
> +	return 0;
> +}
> +console_initcall(dcc_console_init);
> +
> +#define DCC_CONSOLE		&dcc_console
> +#else
> +#define DCC_CONSOLE		NULL
> +#endif
> +
> +static struct uart_driver dcc_reg = {
> +	.owner			= THIS_MODULE,
> +	.driver_name		= SERIAL_DCC_NAME,
> +	.dev_name		= SERIAL_DCC_NAME,
> +	.major			= SERIAL_DCC_MAJOR,
> +	.minor			= SERIAL_DCC_MINOR,
> +	.nr			= UART_NR,
> +	.cons			= DCC_CONSOLE,
> +};
> +
> +static int __init
> +dcc_init(void)
> +{
> +	int i, ret = 0;
> +
> +	printk(KERN_INFO "DCC: JTAG1 Serial emulation driver driver $Revision: 1.1 
> $\n");
> +
> +	ret = uart_register_driver(&dcc_reg);
> +
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < UART_NR; i++)
> +		uart_add_one_port(&dcc_reg, &dcc_ports[i]);

Are you likely to have more than one port?  Does this loop really make
sense?

> +
> +	return ret;
> +}
> +
> +__initcall(dcc_init);
> +
> +MODULE_DESCRIPTION("DCC(JTAG1) JTAG debugger console emulation driver");
> +MODULE_AUTHOR("Hyok S. Choi <hyok.choi@samsung.com>");
> +MODULE_SUPPORTED_DEVICE("ttyJ");
> +MODULE_LICENSE("GPL");

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

RE: [PATCH 2.6.16-git] [SERIAL] Adds DCC(JTAG) serial and the console emulation support

[Hyok S. Choi]

Thank you for your comments.
I'll prepare a revised version ASAP.

Hyok

> -----Original Message-----
> From: Russell King [mailto:rmk@arm.linux.org.uk] On Behalf Of 
> Russell King
> Sent: Monday, April 03, 2006 6:11 AM
> To: Hyok S. Choi
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2.6.16-git] [SERIAL] Adds DCC(JTAG) 
> serial and the console emulation support
> 
> On Wed, Mar 29, 2006 at 04:11:25PM +0900, Hyok S. Choi wrote:
> > diff --git a/drivers/serial/dcc.c b/drivers/serial/dcc.c
> 
> Some comments on this driver below.
> 
> > new file mode 100644
> > index 0000000..d73ccf6
> > --- /dev/null
> > +++ b/drivers/serial/dcc.c
> > @@ -0,0 +1,550 @@
> > +/*
> > + *  linux/drivers/serial/dcc.c
> > + *
> > + *  serial port emulation driver for the JTAG DCC Terminal.
> > + *
> > + * DCC(JTAG1) protocol version for JTAG ICE/ICD Debuggers:
> > + * 	Copyright (C) 2003, 2004, 2005 Hyok S. Choi 
> (hyok.choi@samsung.com)
> > + * 	SAMSUNG ELECTRONICS Co.,Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or 
> > +modify
> > + * it under the terms of the GNU General Public License 
> version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + *  Changelog:
> > + *   Oct-2003 Hyok S. Choi	Created
> > + *   Feb-2004 Hyok S. Choi 	Updated for 
> serial_core.c and 2.6 kernel
> > + *   Apr-2004 Hyok S. Choi 	xmit_string_CR added
> > + *   Mar-2005 Hyok S. Choi	renamed from T32 to DCC
> > + *
> > + */
> > +
> > +#include <linux/config.h>
> > +#include <linux/module.h>
> > +#include <linux/tty.h>
> > +#include <linux/ioport.h>
> > +#include <linux/init.h>
> > +#include <linux/serial.h>
> > +#include <linux/console.h>
> > +#include <linux/sysrq.h>
> > +#include <linux/tty_flip.h>
> > +#include <linux/major.h>
> > +
> > +#include <asm/io.h>
> > +#include <asm/irq.h>
> > +
> > +#include <linux/serial_core.h>
> > +
> > +/*
> > + * if real irq interrupt used for receiving character,
> > + * uncomment below line. Unless polling method id used.
> > + */
> > +//#define DCC_IRQ_USED
> > +
> > +#ifndef  DCC_IRQ_USED
> > +static struct work_struct dcc_poll_task; static void dcc_poll(void 
> > +*data); #endif
> > +
> > +#define UART_NR			1	/* we have only 
> one JTAG port */
> > +
> > +#define SERIAL_DCC_NAME		"ttyJ"
> > +#define SERIAL_DCC_MAJOR	4
> > +#define SERIAL_DCC_MINOR	64
> 
> Is it really a good idea to re-use the same major and minor 
> numbers that are used for 8250?  What if you want to use DCC 
> with a board that has 8250-based serial ports on as well?
> 
> > +
> > +static int __inline__ __check_JTAG_RX_FLAG(void)
> 
> "static inline int" please.
> 
> > +{
> > +	int __ret=0;
> > +	__asm__ __volatile__(
> > +		"	mrc		p14, 0, %0, c0, c0 	
> @ read comms control reg\n"
> > +		"	and		%0, %0, #1		
> @ jtag read buffer status"
> > +		: "=r" (__ret)
> > +		);
> > +
> > +	/* if   __ret 	== 0 : no input yet
> > +				== 1 : a character pending */
> 
> Wouldn't it be clearer to have:
> 
> static inline u32 read_dcc_status(void)
> {
> 	u32 status;
> 
> 	asm("mrc p14, 0, %0, c0, c0" : "=r" (status));
> 
> 	return status;
> }
> 
> and if you want to check the RX flag in the rest of the code:
> 
> 	if (read_dcc_status() & DCC_STAT_RX) {
> 		/* do pending character stuff */
> 	}
> 
> ?
> 
> > +	return __ret;	
> > +}
> > +
> > +static void __inline__ __get_JTAG_RX(volatile char *p) {
> > +	__asm__ __volatile__(
> > +		"	mrc		p14, 0, r3, c1, c0 	
> @ read comms data reg to r5\n"
> > +		"	strb 		r3, 	[%0]		
> @ str a char"
> > +		: /* no output */
> > +		: "r" (p)
> > +		: "r3",  "memory");
> > +}
> 
> Ditto comments above.  The only use of this I can find is:
> 
> 		__get_JTAG_RX(&ch);
> 
> and it's crying out to be:
> 
> static inline char dcc_getchar(void)
> {
> 	char c;
> 
> 	asm("mrc p14, 0, %0, c1, c0" : "=r" (c));
> 
> 	return c;
> }
> 
> ...
> 
> 		ch = dcc_getchar();
> 
> Also, a write_dcc_char(char c) would be useful.
> 
> > +static int __inline__ __check_JTAG_TX_FLAG(void) {
> > +	int __ret=0;
> > +	__asm__ __volatile__(
> > +		"	mrc		p14, 0, %0, c0, c0 	
> @ read comms control reg\n"
> > +		"	and 		%0, %0, #2		
> @ the read buffer status"
> > +		: "=r" (__ret)
> > +		);
> > +
> > +	/* if   __ret 	== 0 : tx is available
> > +				== 2 : tx busy */
> > +	return __ret;	
> > +}
> 
> Same comments as for __check_JTAG_RX_FLAG().
> 
> > +
> > +void xmit_string(char *p, int len)
> > +{
> > +#ifndef CONFIG_JTAG_DCC_OUTPUT_DISABLE
> > +	/*
> > +		r0 = string	; string address
> > +		r1 = 2		; state check bit (write)
> > +		r4 = *string	; character
> > +		r7 = 0		; count
> > +	*/
> > +	__asm__ __volatile__(
> > +		"	mov r7, #0\n"
> > +		"1: 	mrc	p14, 0, r3, c0, c0 	@ read 
> comms control reg\n"
> > +		"	and r3, r3, #2			@ the 
> write buffer status\n"
> > +		"	cmp r3, #2			@ is it 
> available?\n"
> > +		"	beq 1b				@ is 
> not, wait till then\n"
> > +		"	ldrb r4, [%0]			@ load a char\n"
> > +		"	mcr p14, 0, r4, c1, c0		@ write it\n"
> > +		"	add %0, %0, #1			@ str 
> address increase one\n"
> > +		"	add r7, r7, #1			@ count 
> increase\n"
> > +		"	cmp r7, %1			@ 
> compare with str length\n"
> > +		"	bne 1b				@ if it 
> is not yet, loop"
> > +		: /* no output register */
> > +		: "r" (p), "r" (len)
> > +		: "r7", "r3", "r4");
> > +#endif
> > +}
> 
> Does this really need to be written all in assembly?
> Shouldn't it be declared static?
> 
> Wouldn't something like the following be better?
> 
> static void dcc_putchar(struct uart_port *port, char c) {
> 	while (read_dcc_status() & DCC_STAT_TX_FULL)
> 		cpu_relax();
> 	write_dcc_char(c);
> }
> 
> static void xmit_string(struct uart_port *port, const char 
> *p, int len) {
> 	for (; len; len--, p++)
> 		dcc_putchar(port, *p);
> }
> 
> > +
> > +void xmit_string_CR(char *p, int len) { #ifndef 
> > +CONFIG_JTAG_DCC_OUTPUT_DISABLE
> > +	/*
> > +		r0 = string	; string address
> > +		r1 = 2		; state check bit (write)
> > +		r4 = *string	; character
> > +		r7 = 0		; count
> > +	*/
> > +	__asm__ __volatile__(
> > +		"	mov r7, #0\n"
> > +		"	ldrb r4, [%0]			@ load a char\n"
> > +		"1: 	mrc	p14, 0, r3, c0, c0 	@ read 
> comms control reg\n"
> > +		"	and r3, r3, #2			@ the 
> write buffer status\n"
> > +		"	cmp r3, #2			@ is it 
> available?\n"
> > +		"	beq 1b				@ is 
> not, wait till then\n"
> > +		"	mcr p14, 0, r4, c1, c0		@ write it\n"
> > +		"	cmp r4, #0x0a			@ is it LF?\n"
> > +		"	bne 2f				@ if it 
> is not, continue\n"
> > +		"	mov r4, #0x0d			@ set the CR\n"
> > +		"	b   1b				@ loop 
> for writing CR\n"			
> > +		"2:	ldrb r4, [%0, #1]!		@ load a char\n"
> > +		"	add r7, r7, #1			@ count 
> increase\n"
> > +		"	cmp r7, %1			@ 
> compare with str length\n"
> > +		"	bne 1b				@ if it 
> is not yet, loop"
> > +		: /* no output register */
> > +		: "r" (p), "r" (len)
> > +		: "r7", "r3", "r4");
> > +#endif
> > +}
> 
> No need - use uart_console_write() for this, which will do 
> the LF -> CRLF translation for you.  All you need to do is to 
> supply a function to do the individual character based 
> writing.  You can pass dcc_putchar() as the putchar function 
> (which is why I wrote xmit_string that way
> above.)
> 
> > +
> > +
> > +static void
> > +dcc_stop_tx(struct uart_port *port)
> > +{
> > +}
> > +
> > +static inline void
> > +dcc_transmit_buffer(struct uart_port *port) {
> > +	struct circ_buf *xmit = &port->info->xmit;
> > +
> 
> Blank line not required.
> 
> > +	int pendings = uart_circ_chars_pending(xmit);
> > +
> > +	if(pendings + xmit->tail > UART_XMIT_SIZE)
> > +	{
> 
> Interesting indentation style mix.
> 
> > +		xmit_string(&(xmit->buf[xmit->tail]), 
> UART_XMIT_SIZE - xmit->tail);
> > +		xmit_string(&(xmit->buf[0]), xmit->head);
> > +	} else
> > +		xmit_string(&(xmit->buf[xmit->tail]), pendings);
> > +	
> > +	xmit->tail = (xmit->tail + pendings) & (UART_XMIT_SIZE-1);
> > +        port->icount.tx += pendings;
> 
> Tabs vs spaces indentation.  Please use a consistent 
> indentation style.
> 
> > +
> > +	if (uart_circ_empty(xmit))
> > +		dcc_stop_tx(port);
> 
> dcc_stop_tx itself is empty, so this doesn't serve a useful purpose.
> 
> > +}
> > +
> > +static inline void
> > +dcc_transmit_x_char(struct uart_port *port) {
> > +	xmit_string(&port->x_char, 1);
> 
> Another potential user of dcc_putchar().
> 
> > +	port->icount.tx++;
> > +	port->x_char = 0;
> > +}
> > +
> > +static void
> > +dcc_start_tx(struct uart_port *port)
> > +{
> > +    dcc_transmit_buffer(port);
> > +}
> > +
> > +static void
> > +dcc_stop_rx(struct uart_port *port)
> > +{
> > +}
> > +
> > +static void
> > +dcc_enable_ms(struct uart_port *port) { }
> 
> Maybe call all the empty functions which take just a uart_port
> 
> static void dcc_dummy(struct uart_port *port) { }
> 
> and use it in the uart_ops structure as appropriate.
> 
> > +
> > +static inline void
> > +dcc_rx_chars(struct uart_port *port)
> > +{
> > +	unsigned char ch;
> > +	struct tty_struct *tty = port->info->tty;
> > +
> > +	/*
> > +	 * check input.
> > +	 * checking JTAG flag is better to resolve the status test.
> > +	 * incount is NOT used for JTAG1 protocol.
> > +	 */
> > +
> > +	if (__check_JTAG_RX_FLAG())
> > +		/* if   __ret 	== 0 : no input yet
> > +					== 1 : a character pending */
> 
> 	if (read_dcc_status() & DCC_STAT_RX) {
> 
> > +	{
> > +		/* for JTAG 1 protocol, incount is always 1. */
> > +		__get_JTAG_RX(&ch);
> 
> 		ch = dcc_getchar();
> 
> > +
> > +		if (tty) {
> > +			tty_insert_flip_char(tty, ch, TTY_NORMAL);
> > +			port->icount.rx++;
> > +			tty_flip_buffer_push(tty);
> > +		}
> > +	}
> > +}
> > +
> > +static inline void
> > +dcc_overrun_chars(struct uart_port *port) {
> > +	port->icount.overrun++;
> > +}
> > +
> > +static inline void
> > +dcc_tx_chars(struct uart_port *port)
> > +{
> > +	struct circ_buf *xmit = &port->info->xmit;
> > +
> > +	if (port->x_char) {
> > +		dcc_transmit_x_char(port);
> > +		return; 
> > +	}
> > +
> > +	if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
> > +		dcc_stop_tx(port);
> > +		return;
> > +	}
> 
> If we're using interrupt mode, given that dcc_stop_tx() is 
> empty, how do we stop spinning on the transmit interrupt?
> 
> > +
> > +	dcc_transmit_buffer(port);
> > +
> > +	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> > +		uart_write_wakeup(port);
> > +}
> > +
> > +#ifdef DCC_IRQ_USED /* real IRQ used */ static irqreturn_t 
> > +dcc_int(int irq, void *dev_id, struct pt_regs *regs) {
> > +	struct uart_port *port = dev_id;
> > +	int handled = 0;
> > +
> > +	spin_lock(&port->lock);
> > +	
> > +	dcc_rx_chars(port);
> > +	dcc_tx_chars(port);
> > +
> > +	handled = 1;
> > +	spin_unlock(&port->lock);
> > +	
> > +	return IRQ_RETVAL(handled);
> > +}
> > +
> > +#else /* emulation by scheduled work */ static void dcc_poll(void 
> > +*data) {
> > +	struct uart_port *port = data;
> > +
> > +	spin_lock(&port->lock);
> > +	
> > +	dcc_rx_chars(port);
> > +	dcc_tx_chars(port);
> > +
> > +	schedule_delayed_work(&dcc_poll_task, 1);
> > +
> > +	spin_unlock(&port->lock);
> > +	
> > +}
> > +#endif /* end of DCC_IRQ_USED */
> > +
> > +static unsigned int
> > +dcc_tx_empty(struct uart_port *port)
> > +{
> > +	return TIOCSER_TEMT;
> > +}
> > +
> > +static unsigned int
> > +dcc_get_mctrl(struct uart_port *port) {
> > +	return 0;
> 
> Should return TIOCM_CTS | TIOCM_DSR | TIOCM_CD if control 
> lines aren't implemented.
> 
> > +}
> > +
> > +static void
> > +dcc_set_mctrl(struct uart_port *port, unsigned int mctrl) { }
> > +
> > +static void
> > +dcc_break_ctl(struct uart_port *port, int break_state) { }
> > +
> > +static int dcc_startup(struct uart_port *port) { #ifdef 
> DCC_IRQ_USED 
> > +/* real IRQ used */
> > +	int retval;
> > +
> > +	/* Allocate the IRQ */
> > +	retval = request_irq(port->irq, dcc_int, SA_INTERRUPT,
> > +			     "serial_dcc", port);
> > +	if (retval)
> > +		return retval;
> > +#else /* emulation */
> > +	/* Initialize the work, and shcedule it. */
> > +	INIT_WORK(&dcc_poll_task, dcc_poll, port);
> > +	schedule_delayed_work(&dcc_poll_task, 1); #endif
> > +
> > +	return 0;
> > +}
> > +
> > +static void dcc_shutdown(struct uart_port *port) {
> 
> Shouldn't the dcc_poll_task be stopped somehow here?
> 
> > +}
> > +
> > +static void
> > +dcc_set_termios(struct uart_port *port, struct termios *termios,
> > +		   struct termios *old)
> > +{
> > +#ifdef DCC_IRQ_USED
> > +	unsigned long flags;
> > +#endif
> > +	unsigned int baud, quot;
> > +
> > +	/*
> > +	 * We don't support parity, stop bits, or anything other
> > +	 * than 8 bits, so clear these termios flags.
> > +	 */
> > +	termios->c_cflag &= ~(CSIZE | CSTOPB | PARENB | PARODD | CREAD);
> > +	termios->c_cflag |= CS8;
> > +
> > +	/*
> > +	 * We don't appear to support any error conditions either.
> > +	 */
> > +	termios->c_iflag &= ~(INPCK | IGNPAR | IGNBRK | BRKINT);
> > +
> > +	/*
> > +	 * Ask the core to calculate the divisor for us.
> > +	 */
> > +	baud = uart_get_baud_rate(port, termios, old, 0, 
> port->uartclk/16); 
> > +	quot = uart_get_divisor(port, baud);
> > +
> > +#ifdef DCC_IRQ_USED
> > +	spin_lock_irqsave(&port->lock, flags); #endif
> > +
> > +	uart_update_timeout(port, termios->c_cflag, baud);
> > +
> > +#ifdef DCC_IRQ_USED
> > +	spin_unlock_irqrestore(&port->lock, flags); #endif }
> > +
> > +static const char *dcc_type(struct uart_port *port) {
> > +	return port->type == PORT_DCC_JTAG1 ? "DCC" : NULL; }
> > +
> > +static void dcc_release_port(struct uart_port *port) { }
> > +
> > +static int dcc_request_port(struct uart_port *port) {
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Configure/autoconfigure the port.
> > + */
> > +static void dcc_config_port(struct uart_port *port, int flags) {
> > +        if (flags & UART_CONFIG_TYPE) {
> > +                port->type = PORT_DCC_JTAG1;
> > +                dcc_request_port(port);
> > +        }
> > +}
> > +        
> > +/*      
> > + * verify the new serial_struct (for TIOCSSERIAL).
> > + */     
> > +static int dcc_verify_port(struct uart_port *port, struct 
> > +serial_struct *ser) {
> > +        int ret = 0;
> > +        if (ser->type != PORT_UNKNOWN && ser->type != 
> PORT_DCC_JTAG1)
> > +                ret = -EINVAL;
> > +        if (ser->irq < 0 || ser->irq >= NR_IRQS)
> > +                ret = -EINVAL;
> > +        if (ser->baud_base < 9600)
> > +                ret = -EINVAL;
> > +	return ret;
> 
> Weirdo indentation style strikes again.
> 
> > +}
> > +
> > +static struct uart_ops dcc_pops = {
> > +	.tx_empty	= dcc_tx_empty,
> > +	.set_mctrl	= dcc_set_mctrl,
> > +	.get_mctrl	= dcc_get_mctrl,
> > +	.stop_tx	= dcc_stop_tx,
> > +	.start_tx	= dcc_start_tx,
> > +	.stop_rx	= dcc_stop_rx,
> > +	.enable_ms	= dcc_enable_ms,
> > +	.break_ctl	= dcc_break_ctl,
> > +	.startup	= dcc_startup,
> > +	.shutdown	= dcc_shutdown,
> > +	.set_termios	= dcc_set_termios,
> > +	.type		= dcc_type,
> > +	.release_port	= dcc_release_port,
> > +	.request_port	= dcc_request_port,
> > +	.config_port	= dcc_config_port,
> > +	.verify_port	= dcc_verify_port,
> > +};
> > +
> > +static struct uart_port dcc_ports[UART_NR] = {
> 
> If there's only one port, this array isn't required.
> 
> > +	{
> > +		.membase	= (char*)0x12345678,	/* we 
> need these garbages */
> > +		.mapbase	= 0x12345678,		/* for 
> serial_core.c */
> > +		.iotype		= SERIAL_IO_MEM,	
> 
> .iotype should be UPIO_xxx not SERIAL_IO_xxx.
> 
> > +#ifdef DCC_IRQ_USED
> > +		.irq		= INT_N_EXT0,
> > +#else
> > +		.irq		= 0,
> > +#endif
> > +		.uartclk	= 14745600,			 
> > +		.fifosize	= 0,
> > +		.ops		= &dcc_pops,
> > +		.flags		= ASYNC_BOOT_AUTOCONF,
> 
> .flags should be UPF_xxx not ASYNC_xxx.
> 
> > +		.line		= 0,
> > +	},
> > +};
> > +
> > +
> > +#ifdef CONFIG_SERIAL_DCC_CONSOLE
> > +
> > +static void
> > +dcc_console_write(struct console *co, const char *s, unsigned int 
> > +count) {
> > +	xmit_string_CR((char*)s, count);
> > +}
> > +
> > +/*
> > + * Read the current UART setup.
> > + */
> > +static void __init
> > +dcc_console_get_options(struct uart_port *port, int *baud, int 
> > +*parity, int
> > *bits)
> > +{
> > +	*baud = 9600;
> > +	*parity = 'n';
> > +	*bits = 8;
> > +}
> 
> Unnecessary function.
> 
> > +
> > +static int __init
> > +dcc_console_setup(struct console *co, char *options) {
> > +	struct uart_port *port;
> > +	int baud = 9600;
> > +	int bits = 8;
> > +	int parity = 'n';
> > +	int flow = 'n';
> > +
> > +	if (co->index >= UART_NR)
> > +		co->index = 0;
> > +	port = &dcc_ports[co->index];
> 
> If you're not likely to have more than one port, this doesn't 
> make sense.
> 
> > +
> > +	if (options)
> > +		uart_parse_options(options, &baud, &parity, 
> &bits, &flow);
> > +	else
> > +		dcc_console_get_options(port, &baud, &parity, &bits);
> > +
> > +	return uart_set_options(port, co, baud, parity, bits, flow); }
> > +
> > +extern struct uart_driver dcc_reg;
> > +static struct console dcc_console = {
> > +	.name		= SERIAL_DCC_NAME,
> > +	.write		= dcc_console_write,
> > +	.device		= uart_console_device,
> > +	.setup		= dcc_console_setup,
> > +	.flags		= CON_PRINTBUFFER,
> > +	.index		= -1,
> > +	.data		= &dcc_reg,
> > +};
> > +
> > +static int __init dcc_console_init(void) {
> > +	register_console(&dcc_console);
> > +	return 0;
> > +}
> > +console_initcall(dcc_console_init);
> > +
> > +#define DCC_CONSOLE		&dcc_console
> > +#else
> > +#define DCC_CONSOLE		NULL
> > +#endif
> > +
> > +static struct uart_driver dcc_reg = {
> > +	.owner			= THIS_MODULE,
> > +	.driver_name		= SERIAL_DCC_NAME,
> > +	.dev_name		= SERIAL_DCC_NAME,
> > +	.major			= SERIAL_DCC_MAJOR,
> > +	.minor			= SERIAL_DCC_MINOR,
> > +	.nr			= UART_NR,
> > +	.cons			= DCC_CONSOLE,
> > +};
> > +
> > +static int __init
> > +dcc_init(void)
> > +{
> > +	int i, ret = 0;
> > +
> > +	printk(KERN_INFO "DCC: JTAG1 Serial emulation driver driver 
> > +$Revision: 1.1
> > $\n");
> > +
> > +	ret = uart_register_driver(&dcc_reg);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (i = 0; i < UART_NR; i++)
> > +		uart_add_one_port(&dcc_reg, &dcc_ports[i]);
> 
> Are you likely to have more than one port?  Does this loop 
> really make sense?
> 
> > +
> > +	return ret;
> > +}
> > +
> > +__initcall(dcc_init);
> > +
> > +MODULE_DESCRIPTION("DCC(JTAG1) JTAG debugger console emulation 
> > +driver"); MODULE_AUTHOR("Hyok S. Choi <hyok.choi@samsung.com>"); 
> > +MODULE_SUPPORTED_DEVICE("ttyJ"); MODULE_LICENSE("GPL");
> 
> --
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:  2.6 Serial core
> 
>