From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964796AbWDCBBY (ORCPT ); Sun, 2 Apr 2006 21:01:24 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S964797AbWDCBBY (ORCPT ); Sun, 2 Apr 2006 21:01:24 -0400 Received: from mailout1.samsung.com ([203.254.224.24]:25972 "EHLO mailout1.samsung.com") by vger.kernel.org with ESMTP id S964796AbWDCBBX (ORCPT ); Sun, 2 Apr 2006 21:01:23 -0400 Date: Mon, 03 Apr 2006 10:01:24 +0900 From: "Hyok S. Choi" Subject: RE: [PATCH 2.6.16-git] [SERIAL] Adds DCC(JTAG) serial and the console emulation support In-reply-to: <20060402211117.GB13901@flint.arm.linux.org.uk> To: "'Russell King'" Cc: linux-kernel@vger.kernel.org Message-id: <0IX400GAOG5XUN@mmp2.samsung.com> Organization: Samsung Electronics Co.,Ltd. MIME-version: 1.0 X-MIMEOLE: Produced By Microsoft MimeOLE V6.00.2900.2527 X-Mailer: Microsoft Office Outlook, Build 11.0.6353 Content-type: text/plain; charset=us-ascii Content-transfer-encoding: 7BIT Thread-index: AcZWmgGxVnaom+HMTKKU7to25ZmTxAAH80Zg Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > + > > +#include > > + > > +/* > > + * 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 "); > > +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 > >