[PATCH] serial: DCC(JTAG) serial and console emulation support

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

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.

Most of the code was taken from Hyok S. Choi original work, but the
inline assmebly needed some work and updating. It now supports ARMv7.
Also the description above is from Hyok also.

CC: Hyok S. Choi <hyok.choi@samsung.com>
CC: Tony Lindgren <tony@atomide.com>
Signed-off-by: Jeff Ohlstein <johlstei@quicinc.com>
Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 drivers/serial/Kconfig      |   31 +++
 drivers/serial/Makefile     |    1 +
 drivers/serial/dcc.c        |  467 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/serial_core.h |    3 +
 4 files changed, 502 insertions(+), 0 deletions(-)
 create mode 100644 drivers/serial/dcc.c

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 12900f7..24ead62 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -495,6 +495,37 @@ config SERIAL_S3C2400
 	help
 	  Serial port support for the Samsung S3C2400 SoC
 
+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.
+
+	  if unsure, say N.
+
+config SERIAL_DCC_STDSERIAL
+	bool "Install JTAG ICE/ICD DCC as standard serial"
+	default y
+	depends on !SERIAL_8250 && SERIAL_DCC
+	help
+	  Say Y here if you want to install DCC driver as a normal serial port
+	  /dev/ttyS0 (major 4, minor 64). Otherwise, it appears as /dev/ttyJ0
+	  (major 4, minor 128) and can co-exist with other UARTs, such as
+	  8250/16C550 compatibles.
+
 config SERIAL_S3C2410
 	tristate "Samsung S3C2410 Serial port support"
 	depends on SERIAL_SAMSUNG && CPU_S3C2410
diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index 1ca4fd5..896c3f6 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_SERIAL_8250_MCA) += 8250_mca.o
 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_PNX8XXX) += pnx8xxx_uart.o
 obj-$(CONFIG_SERIAL_SA1100) += sa1100.o
diff --git a/drivers/serial/dcc.c b/drivers/serial/dcc.c
new file mode 100644
index 0000000..a1a74d3
--- /dev/null
+++ b/drivers/serial/dcc.c
@@ -0,0 +1,467 @@
+/*
+ *  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 at 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
+ *   Mar-2005 Hyok S. Choi	renamed from T32 to DCC
+ *   Apr-2006 Hyok S. Choi	revised including the MAJOR number
+ *
+ */
+
+#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 <linux/io.h>
+#include <linux/irq.h>
+
+#include <linux/serial_core.h>
+
+#define DCC_POLL_RUN		0
+#define DCC_POLL_STOP		1
+#define DCC_POLL_STOPPED	2
+
+static struct uart_port dcc_port;
+static struct delayed_work dcc_poll_task;
+static void dcc_poll(struct work_struct *work);
+static int dcc_poll_state = DCC_POLL_STOPPED;
+
+#define UART_NR			1	/* we have only one JTAG port */
+
+#ifdef CONFIG_SERIAL_DCC_STDSERIAL
+/* use ttyS for emulation of standard serial driver */
+#define SERIAL_DCC_NAME		"ttyS"
+#define SERIAL_DCC_MINOR	64
+#else
+/* use ttyJ0(128) */
+#define SERIAL_DCC_NAME		"ttyJ"
+#define SERIAL_DCC_MINOR	(64 + 64)
+#endif
+#define SERIAL_DCC_MAJOR	TTY_MAJOR
+
+/* DCC Status Bits */
+#define DCC_STATUS_RX		(1 << 30)
+#define DCC_STATUS_TX		(1 << 29) 
+
+static inline u32
+__dcc_getstatus(void)
+{
+	u32 __ret;
+
+	asm("mrc p14, 0, %0, c0, c1, 0	@ read comms ctrl reg"
+		: "=r" (__ret) : : "cc");
+
+	return __ret;
+}
+
+
+#if !defined(CONFIG_CPU_V7)
+static inline char
+__dcc_getchar(void)
+{
+	char __c;
+
+	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+		: "=r" (__c) : : "cc");
+
+	return __c;
+}
+#else
+static inline char
+__dcc_getchar(void)
+{
+	char __c;
+
+	asm(
+	"get_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
+			bne get_wait                                    \n\
+			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
+		: "=r" (__c) : : "cc");
+
+	return __c;
+}
+#endif
+
+#if !defined(CONFIG_CPU_V7)
+static inline void
+__dcc_putchar(char c)
+{
+	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
+		: /* no output register */
+		: "r" (c) : "cc");
+}
+#else
+static inline void
+__dcc_putchar(char c)
+{
+	asm(
+	"put_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
+			bcs put_wait                                \n\
+			mcr p14, 0, %0, c0, c5, 0                   "
+	: : "r" (c) : "cc");
+}
+#endif
+
+static void
+dcc_putchar(struct uart_port *port, int ch)
+{
+	while (__dcc_getstatus() & DCC_STATUS_TX)
+		cpu_relax();
+	__dcc_putchar((char)(ch & 0xFF));
+}
+
+static inline void
+xmit_string(struct uart_port *port, char *p, int len)
+{
+	for ( ; len; len--, p++)
+		dcc_putchar(port, *p);
+}
+
+static inline void
+dcc_transmit_buffer(struct uart_port *port)
+{
+	struct circ_buf *xmit = &port->state->xmit;
+	int pendings = uart_circ_chars_pending(xmit);
+
+	if(pendings + xmit->tail > UART_XMIT_SIZE)
+	{
+		xmit_string(port, &(xmit->buf[xmit->tail]),
+			UART_XMIT_SIZE - xmit->tail);
+		xmit_string(port, &(xmit->buf[0]), xmit->head);
+	} else
+		xmit_string(port, &(xmit->buf[xmit->tail]), pendings);
+	
+	xmit->tail = (xmit->tail + pendings) & (UART_XMIT_SIZE-1);
+	port->icount.tx += pendings;
+}
+
+static inline void
+dcc_transmit_x_char(struct uart_port *port)
+{
+	dcc_putchar(port, port->x_char);
+	port->icount.tx++;
+	port->x_char = 0;
+}
+
+static void
+dcc_start_tx(struct uart_port *port)
+{
+	dcc_transmit_buffer(port);
+}
+
+static inline void
+dcc_rx_chars(struct uart_port *port)
+{
+	unsigned char ch;
+	struct tty_struct *tty = port->state->port.tty;
+
+	/*
+	 * check input.
+	 * checking JTAG flag is better to resolve the status test.
+	 * incount is NOT used for JTAG1 protocol.
+	 */
+
+	if (__dcc_getstatus() & DCC_STATUS_RX)
+	{
+
+		/* for JTAG 1 protocol, incount is always 1. */
+		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->state->xmit;
+
+	if (port->x_char) {
+		dcc_transmit_x_char(port);
+		return; 
+	}
+
+	if (uart_circ_empty(xmit) || uart_tx_stopped(port))
+		return;
+
+	dcc_transmit_buffer(port);
+
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+		uart_write_wakeup(port);
+}
+
+static void
+dcc_poll(struct work_struct *work)
+{
+	spin_lock(&dcc_port.lock);
+	
+	if (dcc_poll_state != DCC_POLL_RUN) {
+		dcc_poll_state = DCC_POLL_STOPPED;
+		goto dcc_poll_stop;
+	}
+
+	dcc_rx_chars(&dcc_port);
+	dcc_tx_chars(&dcc_port);
+
+	schedule_delayed_work(&dcc_poll_task, 1);
+
+dcc_poll_stop:
+	spin_unlock(&dcc_port.lock);
+}
+
+static unsigned int
+dcc_tx_empty(struct uart_port *port)
+{
+	return TIOCSER_TEMT;
+}
+
+static unsigned int
+dcc_get_mctrl(struct uart_port *port)
+{
+	return TIOCM_CTS | TIOCM_DSR | TIOCM_CD;
+}
+
+static int
+dcc_startup(struct uart_port *port)
+{
+	/* Initialize the work, and shcedule it. */
+	INIT_DELAYED_WORK(&dcc_poll_task, dcc_poll);
+	spin_lock(&port->lock);
+	if (dcc_poll_state != DCC_POLL_RUN)
+		dcc_poll_state = DCC_POLL_RUN;
+	schedule_delayed_work(&dcc_poll_task, 1);
+	spin_unlock(&port->lock);
+
+	return 0;
+}
+
+static void
+dcc_shutdown(struct uart_port *port)
+{
+	spin_lock(&port->lock);
+	dcc_poll_state = DCC_POLL_STOP;
+	spin_unlock(&port->lock);
+}
+
+static void
+dcc_set_termios(struct uart_port *port, struct ktermios *termios,
+		   struct ktermios *old)
+{
+	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);
+
+	uart_update_timeout(port, termios->c_cflag, baud);
+
+}
+
+static const char *
+dcc_type(struct uart_port *port)
+{
+	return port->type == PORT_DCC_JTAG1 ? "DCC" : NULL;
+}
+
+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;
+}
+
+/* dummy operation handlers for uart_ops */
+static void
+dcc_dummy_ops(struct uart_port *port)
+{
+}
+static void
+dcc_dummy_ops_ui(struct uart_port *port, unsigned int ui)
+{
+}
+static void
+dcc_dummy_ops_i(struct uart_port *port, int i)
+{
+}
+
+static struct uart_ops dcc_pops = {
+	.tx_empty	= dcc_tx_empty,
+	.set_mctrl	= dcc_dummy_ops_ui,
+	.get_mctrl	= dcc_get_mctrl,
+	.stop_tx	= dcc_dummy_ops,
+	.start_tx	= dcc_start_tx,
+	.stop_rx	= dcc_dummy_ops,
+	.enable_ms	= dcc_dummy_ops,
+	.break_ctl	= dcc_dummy_ops_i,
+	.startup	= dcc_startup,
+	.shutdown	= dcc_shutdown,
+	.set_termios	= dcc_set_termios,
+	.type		= dcc_type,
+	.release_port	= dcc_dummy_ops,
+	.request_port	= dcc_request_port,
+	.config_port	= dcc_config_port,
+	.verify_port	= dcc_verify_port,
+};
+
+static struct uart_port dcc_port = {
+	.membase	= (char*)0x12345678,	/* we need these garbages */
+	.mapbase	= 0x12345678,		/* for serial_core.c */
+	.iotype		= UPIO_MEM,	
+#ifdef DCC_IRQ_USED
+	.irq		= DCC_IRQ,
+#else
+	.irq		= 0,
+#endif
+	.uartclk	= 14745600,
+	.fifosize	= 0,
+	.ops		= &dcc_pops,
+	.flags		= UPF_BOOT_AUTOCONF,
+	.line		= 0,
+};
+
+#ifdef CONFIG_SERIAL_DCC_CONSOLE
+static void
+dcc_console_write(struct console *co, const char *s, unsigned int count)
+{
+	uart_console_write(&dcc_port, s, count, dcc_putchar);
+}
+
+static int __init
+dcc_console_setup(struct console *co, char *options)
+{
+	struct uart_port *port = &dcc_port;
+	int baud = 9600;
+	int bits = 8;
+	int parity = 'n';
+	int flow = 'n';
+
+	if (options)
+		uart_parse_options(options, &baud, &parity, &bits, &flow);
+
+	return uart_set_options(port, co, baud, parity, bits, flow);
+}
+
+static 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 ret;
+
+	printk(KERN_INFO "DCC: JTAG1 Serial emulation driver driver $Revision: 1.1 $\n");
+
+	ret = uart_register_driver(&dcc_reg);
+
+	if (ret)
+		return ret;
+
+	uart_add_one_port(&dcc_reg, &dcc_port);
+
+	return 0;
+}
+
+__initcall(dcc_init);
+
+MODULE_DESCRIPTION("DCC(JTAG1) JTAG debugger console emulation driver");
+MODULE_AUTHOR("Hyok S. Choi <hyok.choi@samsung.com>");
+MODULE_SUPPORTED_DEVICE(SERIAL_DCC_NAME);
+MODULE_LICENSE("GPL");
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 563e234..a360c3a 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -196,6 +196,9 @@
 /* High Speed UART for Medfield */
 #define PORT_MFD	95
 
+/* DCC(JTAG) emulation port types */
+#define PORT_DCC_JTAG1	96
+
 #ifdef __KERNEL__
 
 #include <linux/compiler.h>
-- 
1.7.1

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Tue, 5 Oct 2010, Daniel Walker wrote:

> +#if !defined(CONFIG_CPU_V7)
> +static inline char
> +__dcc_getchar(void)
> +{
> +	char __c;
> +
> +	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> +		: "=r" (__c) : : "cc");
> +
> +	return __c;
> +}
> +#else
> +static inline char
> +__dcc_getchar(void)
> +{
> +	char __c;
> +
> +	asm(
> +	"get_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> +			bne get_wait                                    \n\
> +			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> +		: "=r" (__c) : : "cc");
> +
> +	return __c;
> +}
> +#endif
> +
> +#if !defined(CONFIG_CPU_V7)
> +static inline void
> +__dcc_putchar(char c)
> +{
> +	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
> +		: /* no output register */
> +		: "r" (c) : "cc");
> +}
> +#else
> +static inline void
> +__dcc_putchar(char c)
> +{
> +	asm(
> +	"put_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> +			bcs put_wait                                \n\
> +			mcr p14, 0, %0, c0, c5, 0                   "
> +	: : "r" (c) : "cc");
> +}
> +#endif

Please move the #ifdef conditionals inside the respective functions so 
to have only one function pair with the various alternatives embedded 
into them.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Tue, 2010-10-05 at 22:55 -0400, Nicolas Pitre wrote:
> On Tue, 5 Oct 2010, Daniel Walker wrote:
> 
> > +#if !defined(CONFIG_CPU_V7)
> > +static inline char
> > +__dcc_getchar(void)
> > +{
> > +	char __c;
> > +
> > +	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > +		: "=r" (__c) : : "cc");
> > +
> > +	return __c;
> > +}
> > +#else
> > +static inline char
> > +__dcc_getchar(void)
> > +{
> > +	char __c;
> > +
> > +	asm(
> > +	"get_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> > +			bne get_wait                                    \n\
> > +			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > +		: "=r" (__c) : : "cc");
> > +
> > +	return __c;
> > +}
> > +#endif
> > +
> > +#if !defined(CONFIG_CPU_V7)
> > +static inline void
> > +__dcc_putchar(char c)
> > +{
> > +	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
> > +		: /* no output register */
> > +		: "r" (c) : "cc");
> > +}
> > +#else
> > +static inline void
> > +__dcc_putchar(char c)
> > +{
> > +	asm(
> > +	"put_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> > +			bcs put_wait                                \n\
> > +			mcr p14, 0, %0, c0, c5, 0                   "
> > +	: : "r" (c) : "cc");
> > +}
> > +#endif
> 
> Please move the #ifdef conditionals inside the respective functions so 
> to have only one function pair with the various alternatives embedded 
> into them.

My typical clean up policy is do to the inverse of what your suggesting.
Mainly because that's the method that I see used extensive in generic
parts of the kernel.

>From my perspective there are pluses an minuses to both. Your method
reduces lines, and duplication. My method makes the code easier to read.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Wed, 6 Oct 2010, Daniel Walker wrote:

> On Tue, 2010-10-05 at 22:55 -0400, Nicolas Pitre wrote:
> > On Tue, 5 Oct 2010, Daniel Walker wrote:
> > 
> > > +#if !defined(CONFIG_CPU_V7)
> > > +static inline char
> > > +__dcc_getchar(void)
> > > +{
> > > +	char __c;
> > > +
> > > +	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > > +		: "=r" (__c) : : "cc");
> > > +
> > > +	return __c;
> > > +}
> > > +#else
> > > +static inline char
> > > +__dcc_getchar(void)
> > > +{
> > > +	char __c;
> > > +
> > > +	asm(
> > > +	"get_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> > > +			bne get_wait                                    \n\
> > > +			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > > +		: "=r" (__c) : : "cc");
> > > +
> > > +	return __c;
> > > +}
> > > +#endif
> > > +
> > > +#if !defined(CONFIG_CPU_V7)
> > > +static inline void
> > > +__dcc_putchar(char c)
> > > +{
> > > +	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
> > > +		: /* no output register */
> > > +		: "r" (c) : "cc");
> > > +}
> > > +#else
> > > +static inline void
> > > +__dcc_putchar(char c)
> > > +{
> > > +	asm(
> > > +	"put_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> > > +			bcs put_wait                                \n\
> > > +			mcr p14, 0, %0, c0, c5, 0                   "
> > > +	: : "r" (c) : "cc");
> > > +}
> > > +#endif
> > 
> > Please move the #ifdef conditionals inside the respective functions so 
> > to have only one function pair with the various alternatives embedded 
> > into them.
> 
> My typical clean up policy is do to the inverse of what your suggesting.
> Mainly because that's the method that I see used extensive in generic
> parts of the kernel.

Do you have an example?  I don't see such thing in generic code, unless 
two versions of the same function are totally different.  In this case 
you have only the inner inline asm code that is different.

> From my perspective there are pluses an minuses to both. Your method
> reduces lines, and duplication. My method makes the code easier to read.

I disagree.  In reviewing your patch I had to go back and forth between 
the different versions just to figure out what was actually different to 
justify this #ifdef in the first place.  If the #ifdef..#endif was 
surrounding only the different inline asm statements then the difference 
would have been more obvious.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-06 at 10:22 -0400, Nicolas Pitre wrote:
> On Wed, 6 Oct 2010, Daniel Walker wrote:
> 
> > On Tue, 2010-10-05 at 22:55 -0400, Nicolas Pitre wrote:
> > > On Tue, 5 Oct 2010, Daniel Walker wrote:
> > > 
> > > > +#if !defined(CONFIG_CPU_V7)
> > > > +static inline char
> > > > +__dcc_getchar(void)
> > > > +{
> > > > +	char __c;
> > > > +
> > > > +	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > > > +		: "=r" (__c) : : "cc");
> > > > +
> > > > +	return __c;
> > > > +}
> > > > +#else
> > > > +static inline char
> > > > +__dcc_getchar(void)
> > > > +{
> > > > +	char __c;
> > > > +
> > > > +	asm(
> > > > +	"get_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> > > > +			bne get_wait                                    \n\
> > > > +			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > > > +		: "=r" (__c) : : "cc");
> > > > +
> > > > +	return __c;
> > > > +}
> > > > +#endif
> > > > +
> > > > +#if !defined(CONFIG_CPU_V7)
> > > > +static inline void
> > > > +__dcc_putchar(char c)
> > > > +{
> > > > +	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
> > > > +		: /* no output register */
> > > > +		: "r" (c) : "cc");
> > > > +}
> > > > +#else
> > > > +static inline void
> > > > +__dcc_putchar(char c)
> > > > +{
> > > > +	asm(
> > > > +	"put_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> > > > +			bcs put_wait                                \n\
> > > > +			mcr p14, 0, %0, c0, c5, 0                   "
> > > > +	: : "r" (c) : "cc");
> > > > +}
> > > > +#endif
> > > 
> > > Please move the #ifdef conditionals inside the respective functions so 
> > > to have only one function pair with the various alternatives embedded 
> > > into them.
> > 
> > My typical clean up policy is do to the inverse of what your suggesting.
> > Mainly because that's the method that I see used extensive in generic
> > parts of the kernel.
> 
> Do you have an example?  I don't see such thing in generic code, unless 
> two versions of the same function are totally different.  In this case 
> you have only the inner inline asm code that is different.

These may not be the best examples but in include/linux/workqueue.h line
150, work_static() for instance if fully inside an ifdef. There is one
other function example in that file, an one macro version. Also some in
include/linux/ftrace.h , but in that case either the functions do
something or nothing. ftrace.h is a little bit confsued, it has example
of it your way and my way.

It doesn't appear to be a constant .. I see it your way and my way.

> > From my perspective there are pluses an minuses to both. Your method
> > reduces lines, and duplication. My method makes the code easier to read.
> 
> I disagree.  In reviewing your patch I had to go back and forth between 
> the different versions just to figure out what was actually different to 
> justify this #ifdef in the first place.  If the #ifdef..#endif was 
> surrounding only the different inline asm statements then the difference 
> would have been more obvious.

You would have had to go back and forth either way , wouldn't you? In
this case the functions are actually totally different.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Wed, 6 Oct 2010, Daniel Walker wrote:

> On Wed, 2010-10-06 at 10:22 -0400, Nicolas Pitre wrote:
> > On Wed, 6 Oct 2010, Daniel Walker wrote:
> > 
> > > From my perspective there are pluses an minuses to both. Your method
> > > reduces lines, and duplication. My method makes the code easier to read.
> > 
> > I disagree.  In reviewing your patch I had to go back and forth between 
> > the different versions just to figure out what was actually different to 
> > justify this #ifdef in the first place.  If the #ifdef..#endif was 
> > surrounding only the different inline asm statements then the difference 
> > would have been more obvious.
> 
> You would have had to go back and forth either way , wouldn't you? In
> this case the functions are actually totally different.

static inline char
__dcc_getchar(void)
{
     char __c;

#if !defined(CONFIG_CPU_V7)
     asm("mrc p14, 0, %0, c0, c5, 0  @ read comms data reg"
             : "=r" (__c) : : "cc");
#else
     asm(
     "get_wait:      mrc p14, 0, pc, c0, c1, 0                       \n\
                     bne get_wait                                    \n\
                     mrc p14, 0, %0, c0, c5, 0       @ read comms data reg"
             : "=r" (__c) : : "cc");
#endif

     return __c;
}

static inline void
__dcc_putchar(char c)
{
#if !defined(CONFIG_CPU_V7)
     asm("mcr p14, 0, %0, c0, c5, 0  @ write a char"
             : /* no output register */
             : "r" (c) : "cc");
#else
     asm(
     "put_wait:      mrc p14, 0, pc, c0, c1, 0                   \n\
                     bcs put_wait                                \n\
                     mcr p14, 0, %0, c0, c5, 0                   "
     : : "r" (c) : "cc");
#endif
}

To me the above is easier to read.  Not a big deal since the functions 
are rather small, but still an improvement.  Searching for __dcc_putchar 
would produce a single hit, and if the prototype has to change it is 
done in only one place, etc.

BTW the cc clobber in the asm for __dcc_putchar() is unneeded.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-06 at 11:21 -0400, Nicolas Pitre wrote:

> static inline char
> __dcc_getchar(void)
> {
>      char __c;
> 
> #if !defined(CONFIG_CPU_V7)
>      asm("mrc p14, 0, %0, c0, c5, 0  @ read comms data reg"
>              : "=r" (__c) : : "cc");
> #else
>      asm(
>      "get_wait:      mrc p14, 0, pc, c0, c1, 0                       \n\
>                      bne get_wait                                    \n\
>                      mrc p14, 0, %0, c0, c5, 0       @ read comms data reg"
>              : "=r" (__c) : : "cc");
> #endif
> 
>      return __c;
> }
> 
> static inline void
> __dcc_putchar(char c)
> {
> #if !defined(CONFIG_CPU_V7)
>      asm("mcr p14, 0, %0, c0, c5, 0  @ write a char"
>              : /* no output register */
>              : "r" (c) : "cc");
> #else
>      asm(
>      "put_wait:      mrc p14, 0, pc, c0, c1, 0                   \n\
>                      bcs put_wait                                \n\
>                      mcr p14, 0, %0, c0, c5, 0                   "
>      : : "r" (c) : "cc");
> #endif
> }
> 
> To me the above is easier to read.  Not a big deal since the functions 
> are rather small, but still an improvement.  Searching for __dcc_putchar 
> would produce a single hit, and if the prototype has to change it is 
> done in only one place, etc.

It makes the internals of the function much more busy. There's more
stuff that could be executing that you have to think about while
reviewing the function.

> BTW the cc clobber in the asm for __dcc_putchar() is unneeded.

Without it caused a crash in __dcc_getchar() ..

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Wed, 6 Oct 2010, Daniel Walker wrote:

> On Wed, 2010-10-06 at 11:21 -0400, Nicolas Pitre wrote:
> 
> > static inline char
> > __dcc_getchar(void)
> > {
> >      char __c;
> > 
> > #if !defined(CONFIG_CPU_V7)
> >      asm("mrc p14, 0, %0, c0, c5, 0  @ read comms data reg"
> >              : "=r" (__c) : : "cc");
> > #else
> >      asm(
> >      "get_wait:      mrc p14, 0, pc, c0, c1, 0                       \n\
> >                      bne get_wait                                    \n\
> >                      mrc p14, 0, %0, c0, c5, 0       @ read comms data reg"
> >              : "=r" (__c) : : "cc");
> > #endif
> > 
> >      return __c;
> > }
> > 
> > static inline void
> > __dcc_putchar(char c)
> > {
> > #if !defined(CONFIG_CPU_V7)
> >      asm("mcr p14, 0, %0, c0, c5, 0  @ write a char"
> >              : /* no output register */
> >              : "r" (c) : "cc");
> > #else
> >      asm(
> >      "put_wait:      mrc p14, 0, pc, c0, c1, 0                   \n\
> >                      bcs put_wait                                \n\
> >                      mcr p14, 0, %0, c0, c5, 0                   "
> >      : : "r" (c) : "cc");
> > #endif
> > }
> > 
> > To me the above is easier to read.  Not a big deal since the functions 
> > are rather small, but still an improvement.  Searching for __dcc_putchar 
> > would produce a single hit, and if the prototype has to change it is 
> > done in only one place, etc.
> 
> It makes the internals of the function much more busy. There's more
> stuff that could be executing that you have to think about while
> reviewing the function.

I still stand by my opinion that the above is better.  Feel free to 
ignore me.

> > BTW the cc clobber in the asm for __dcc_putchar() is unneeded.
> 
> Without it caused a crash in __dcc_getchar() ..

It is the wrong fix nevertheless.  And in this case it isn't a question 
of opinion.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-06 at 11:47 -0400, Nicolas Pitre wrote:

> It is the wrong fix nevertheless.  And in this case it isn't a question 
> of opinion.

I'm not saying your wrong, I'm sure you know more about it than I do. I
was just letting you know why I added it .

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Wed, 6 Oct 2010, Daniel Walker wrote:

> On Wed, 2010-10-06 at 11:47 -0400, Nicolas Pitre wrote:
> 
> > It is the wrong fix nevertheless.  And in this case it isn't a question 
> > of opinion.
> 
> I'm not saying your wrong, I'm sure you know more about it than I do. I
> was just letting you know why I added it .

Sure.  However it is best to _understand_ why such things may apparently 
fix things.  In this case it would have been by accident, and the code 
could be broken again with a different gcc version.

Adding "cc" in the clobber list is needed only when the asm code is 
modifying the condition flags.

I'd suggest you look at the disassembly difference with and without it. 
My guess is that the whole thing gets optimized away as there is no 
dependencies to be dependent on, in which case the proper fix would be 
to mark it with "volatile".


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-06 at 12:22 -0400, Nicolas Pitre wrote:
> On Wed, 6 Oct 2010, Daniel Walker wrote:
> 
> > On Wed, 2010-10-06 at 11:47 -0400, Nicolas Pitre wrote:
> > 
> > > It is the wrong fix nevertheless.  And in this case it isn't a question 
> > > of opinion.
> > 
> > I'm not saying your wrong, I'm sure you know more about it than I do. I
> > was just letting you know why I added it .
> 
> Sure.  However it is best to _understand_ why such things may apparently 
> fix things.  In this case it would have been by accident, and the code 
> could be broken again with a different gcc version.
> 
> Adding "cc" in the clobber list is needed only when the asm code is 
> modifying the condition flags.
> 
> I'd suggest you look at the disassembly difference with and without it. 
> My guess is that the whole thing gets optimized away as there is no 
> dependencies to be dependent on, in which case the proper fix would be 
> to mark it with "volatile".

Are you saying it's not needed for either the __dcc_putchar or
__dcc_getchar ?

With __dcc_getchar() Jeff O. (who is CC'd) discovered that the assembly
was setup in a way that assumed the condition bits were not touched, but
since they did get touched we ended up missing a branch in dcc_rx_chars
for checking if "tty" is set. Since we're branching in the assembly I'm
assuming we must be setting condition bits with the mrc instruction. 

So for __dcc_getchar it seems like it for sure is need .. 

For  __dcc_putchar I add "cc" also because again we're branching, so I'm
assuming we're also setting condition bits.

Daniel


-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Wed, 6 Oct 2010, Daniel Walker wrote:

> On Wed, 2010-10-06 at 12:22 -0400, Nicolas Pitre wrote:
> > On Wed, 6 Oct 2010, Daniel Walker wrote:
> > 
> > > On Wed, 2010-10-06 at 11:47 -0400, Nicolas Pitre wrote:
> > > 
> > > > It is the wrong fix nevertheless.  And in this case it isn't a question 
> > > > of opinion.
> > > 
> > > I'm not saying your wrong, I'm sure you know more about it than I do. I
> > > was just letting you know why I added it .
> > 
> > Sure.  However it is best to _understand_ why such things may apparently 
> > fix things.  In this case it would have been by accident, and the code 
> > could be broken again with a different gcc version.
> > 
> > Adding "cc" in the clobber list is needed only when the asm code is 
> > modifying the condition flags.
> > 
> > I'd suggest you look at the disassembly difference with and without it. 
> > My guess is that the whole thing gets optimized away as there is no 
> > dependencies to be dependent on, in which case the proper fix would be 
> > to mark it with "volatile".
> 
> Are you saying it's not needed for either the __dcc_putchar or
> __dcc_getchar ?

I'm saying that "CC" is unneeded in those cases where the condition flag 
is unmodified.  Those are the !CONFIG_CPU_V7 cases.

> With __dcc_getchar() Jeff O. (who is CC'd) discovered that the assembly
> was setup in a way that assumed the condition bits were not touched, but
> since they did get touched we ended up missing a branch in dcc_rx_chars
> for checking if "tty" is set. Since we're branching in the assembly I'm
> assuming we must be setting condition bits with the mrc instruction. 

Yep.

> So for __dcc_getchar it seems like it for sure is need .. 
> 
> For  __dcc_putchar I add "cc" also because again we're branching, so I'm
> assuming we're also setting condition bits.

I was talking about the non-branching cases.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-06 at 13:02 -0400, Nicolas Pitre wrote: 
> > Are you saying it's not needed for either the __dcc_putchar or
> > __dcc_getchar ?
> 
> I'm saying that "CC" is unneeded in those cases where the condition flag 
> is unmodified.  Those are the !CONFIG_CPU_V7 cases.

Ok .. I can drop that stuff then.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Tony Lindgren]

Hi,

* Daniel Walker <dwalker@codeaurora.org> [101005 11:59]:
> +#if !defined(CONFIG_CPU_V7)
> +static inline char
> +__dcc_getchar(void)
> +{
> +	char __c;
> +
> +	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> +		: "=r" (__c) : : "cc");
> +
> +	return __c;
> +}
> +#else
> +static inline char
> +__dcc_getchar(void)
> +{
> +	char __c;
> +
> +	asm(
> +	"get_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> +			bne get_wait                                    \n\
> +			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> +		: "=r" (__c) : : "cc");
> +
> +	return __c;
> +}
> +#endif
> +
> +#if !defined(CONFIG_CPU_V7)
> +static inline void
> +__dcc_putchar(char c)
> +{
> +	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
> +		: /* no output register */
> +		: "r" (c) : "cc");
> +}
> +#else
> +static inline void
> +__dcc_putchar(char c)
> +{
> +	asm(
> +	"put_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> +			bcs put_wait                                \n\
> +			mcr p14, 0, %0, c0, c5, 0                   "
> +	: : "r" (c) : "cc");
> +}
> +#endif

Can you please pass the read and write functions to the driver
in platform_data? We are already booting kernels with both
ARMv6 and 7 compiled in.

Also, as it's a driver, other architectures may want to use it too.

Regards,

Tony

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Thu, 2010-10-07 at 14:27 -0700, Tony Lindgren wrote:

> Can you please pass the read and write functions to the driver
> in platform_data? We are already booting kernels with both
> ARMv6 and 7 compiled in.

What kind of situation did you want to use it in ? I was thinking about
how arm could have these functions in a header file, that way we
wouldn't duplicate in multiple places.

> Also, as it's a driver, other architectures may want to use it too.

I'm not sure if this model fits other architectures tho. They may not
have the same read/write/status sorts of stuff. Atleast I don't know
enough about other architectures jtags .

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Thu, 7 Oct 2010, Daniel Walker wrote:

> On Thu, 2010-10-07 at 14:27 -0700, Tony Lindgren wrote:
> 
> > Can you please pass the read and write functions to the driver
> > in platform_data? We are already booting kernels with both
> > ARMv6 and 7 compiled in.
> 
> What kind of situation did you want to use it in ? I was thinking about
> how arm could have these functions in a header file, that way we
> wouldn't duplicate in multiple places.
> 
> > Also, as it's a driver, other architectures may want to use it too.
> 
> I'm not sure if this model fits other architectures tho. They may not
> have the same read/write/status sorts of stuff. Atleast I don't know
> enough about other architectures jtags .

This is ARM specific, and even not all ARM implementations have it.
 

Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Tony Lindgren]

* Nicolas Pitre <nico@fluxnic.net> [101007 18:19]:
> On Thu, 7 Oct 2010, Daniel Walker wrote:
> 
> > On Thu, 2010-10-07 at 14:27 -0700, Tony Lindgren wrote:
> > 
> > > Can you please pass the read and write functions to the driver
> > > in platform_data? We are already booting kernels with both
> > > ARMv6 and 7 compiled in.
> > 
> > What kind of situation did you want to use it in ? I was thinking about
> > how arm could have these functions in a header file, that way we
> > wouldn't duplicate in multiple places.
> > 
> > > Also, as it's a driver, other architectures may want to use it too.
> > 
> > I'm not sure if this model fits other architectures tho. They may not
> > have the same read/write/status sorts of stuff. Atleast I don't know
> > enough about other architectures jtags .
> 
> This is ARM specific, and even not all ARM implementations have it.

Are you sure? I thought DCC is JTAG specific standard?

Tony

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [101008 13:27]:
> * Nicolas Pitre <nico@fluxnic.net> [101007 18:19]:
> > On Thu, 7 Oct 2010, Daniel Walker wrote:
> > 
> > > On Thu, 2010-10-07 at 14:27 -0700, Tony Lindgren wrote:
> > > 
> > > > Can you please pass the read and write functions to the driver
> > > > in platform_data? We are already booting kernels with both
> > > > ARMv6 and 7 compiled in.
> > > 
> > > What kind of situation did you want to use it in ? I was thinking about
> > > how arm could have these functions in a header file, that way we
> > > wouldn't duplicate in multiple places.
> > > 
> > > > Also, as it's a driver, other architectures may want to use it too.
> > > 
> > > I'm not sure if this model fits other architectures tho. They may not
> > > have the same read/write/status sorts of stuff. Atleast I don't know
> > > enough about other architectures jtags .
> > 
> > This is ARM specific, and even not all ARM implementations have it.
> 
> Are you sure? I thought DCC is JTAG specific standard?

Never mind, it seems like ARM specific scan chain.

Tony

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Fri, 8 Oct 2010, Tony Lindgren wrote:

> * Nicolas Pitre <nico@fluxnic.net> [101007 18:19]:
> > On Thu, 7 Oct 2010, Daniel Walker wrote:
> > 
> > > On Thu, 2010-10-07 at 14:27 -0700, Tony Lindgren wrote:
> > > 
> > > > Can you please pass the read and write functions to the driver
> > > > in platform_data? We are already booting kernels with both
> > > > ARMv6 and 7 compiled in.
> > > 
> > > What kind of situation did you want to use it in ? I was thinking about
> > > how arm could have these functions in a header file, that way we
> > > wouldn't duplicate in multiple places.
> > > 
> > > > Also, as it's a driver, other architectures may want to use it too.
> > > 
> > > I'm not sure if this model fits other architectures tho. They may not
> > > have the same read/write/status sorts of stuff. Atleast I don't know
> > > enough about other architectures jtags .
> > 
> > This is ARM specific, and even not all ARM implementations have it.
> 
> Are you sure?

Certainly.

> I thought DCC is JTAG specific standard?

Not at all.  JTAG standard is a bit like Ethernet while DCC would be 
HTTP.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Tony Lindgren]

* Daniel Walker <dwalker@codeaurora.org> [101007 14:50]:
> On Thu, 2010-10-07 at 14:27 -0700, Tony Lindgren wrote:
> 
> > Can you please pass the read and write functions to the driver
> > in platform_data? We are already booting kernels with both
> > ARMv6 and 7 compiled in.
> 
> What kind of situation did you want to use it in ? I was thinking about
> how arm could have these functions in a header file, that way we
> wouldn't duplicate in multiple places.

Well with ifdef else it's still either or for kernels that have
ARMv6 and 7 both compiled in. It's best to initialize those
functions dynamically.

Regards,

Tony

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Thu, 7 Oct 2010, Tony Lindgren wrote:

> Hi,
> 
> * Daniel Walker <dwalker@codeaurora.org> [101005 11:59]:
> > +#if !defined(CONFIG_CPU_V7)
> > +static inline char
> > +__dcc_getchar(void)
> > +{
> > +	char __c;
> > +
> > +	asm("mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > +		: "=r" (__c) : : "cc");
> > +
> > +	return __c;
> > +}
> > +#else
> > +static inline char
> > +__dcc_getchar(void)
> > +{
> > +	char __c;
> > +
> > +	asm(
> > +	"get_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> > +			bne get_wait                                    \n\
> > +			mrc p14, 0, %0, c0, c5, 0	@ read comms data reg"
> > +		: "=r" (__c) : : "cc");
> > +
> > +	return __c;
> > +}
> > +#endif
> > +
> > +#if !defined(CONFIG_CPU_V7)
> > +static inline void
> > +__dcc_putchar(char c)
> > +{
> > +	asm("mcr p14, 0, %0, c0, c5, 0	@ write a char"
> > +		: /* no output register */
> > +		: "r" (c) : "cc");
> > +}
> > +#else
> > +static inline void
> > +__dcc_putchar(char c)
> > +{
> > +	asm(
> > +	"put_wait:	mrc p14, 0, pc, c0, c1, 0                   \n\
> > +			bcs put_wait                                \n\
> > +			mcr p14, 0, %0, c0, c5, 0                   "
> > +	: : "r" (c) : "cc");
> > +}
> > +#endif
> 
> Can you please pass the read and write functions to the driver
> in platform_data? We are already booting kernels with both
> ARMv6 and 7 compiled in.

No.  This has nothing to do with platform as this can be determined 
within the driver itself.  Would be much better to simply determine 
which flavor to use at driver init time and assign two function pointers.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Tony Lindgren]

* Nicolas Pitre <nico@fluxnic.net> [101007 18:16]:
> On Thu, 7 Oct 2010, Tony Lindgren wrote:
> > 
> > Can you please pass the read and write functions to the driver
> > in platform_data? We are already booting kernels with both
> > ARMv6 and 7 compiled in.
> 
> No.  This has nothing to do with platform as this can be determined 
> within the driver itself.  Would be much better to simply determine 
> which flavor to use at driver init time and assign two function pointers.

In the long run some platform init code is needed for powering
up the JTAG interface and take care of pin multiplexing etc.

Also, isn't DCC (Debug Communications Channel) a JTAG standard? At least
the following does not say anything about DCC being ARM specific:

http://en.wikipedia.org/wiki/Joint_Test_Action_Group

BTW, we already have ETM (Embedded Trace Module) in arch/arm/kernel/etm.c.
That is set up as amba driver.

Regards,

Tony

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [101008 13:24]:
> * Nicolas Pitre <nico@fluxnic.net> [101007 18:16]:
> > On Thu, 7 Oct 2010, Tony Lindgren wrote:
> > > 
> > > Can you please pass the read and write functions to the driver
> > > in platform_data? We are already booting kernels with both
> > > ARMv6 and 7 compiled in.
> > 
> > No.  This has nothing to do with platform as this can be determined 
> > within the driver itself.  Would be much better to simply determine 
> > which flavor to use at driver init time and assign two function pointers.
> 
> In the long run some platform init code is needed for powering
> up the JTAG interface and take care of pin multiplexing etc.
> 
> Also, isn't DCC (Debug Communications Channel) a JTAG standard? At least
> the following does not say anything about DCC being ARM specific:
> 
> http://en.wikipedia.org/wiki/Joint_Test_Action_Group
> 
> BTW, we already have ETM (Embedded Trace Module) in arch/arm/kernel/etm.c.
> That is set up as amba driver.

Hmm, then again it says this about the scan chains:

  "SCAN_N ... ARM instruction to select the numbered scan chain used
  with EXTEST or INTEST. There are six scan chains"

Which seems like these scan chains are ARM specific.

Regards,

Tony

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Fri, 8 Oct 2010, Tony Lindgren wrote:

> * Tony Lindgren <tony@atomide.com> [101008 13:24]:
> > * Nicolas Pitre <nico@fluxnic.net> [101007 18:16]:
> > > On Thu, 7 Oct 2010, Tony Lindgren wrote:
> > > > 
> > > > Can you please pass the read and write functions to the driver
> > > > in platform_data? We are already booting kernels with both
> > > > ARMv6 and 7 compiled in.
> > > 
> > > No.  This has nothing to do with platform as this can be determined 
> > > within the driver itself.  Would be much better to simply determine 
> > > which flavor to use at driver init time and assign two function pointers.
> > 
> > In the long run some platform init code is needed for powering
> > up the JTAG interface and take care of pin multiplexing etc.
> > 
> > Also, isn't DCC (Debug Communications Channel) a JTAG standard? At least
> > the following does not say anything about DCC being ARM specific:
> > 
> > http://en.wikipedia.org/wiki/Joint_Test_Action_Group
> > 
> > BTW, we already have ETM (Embedded Trace Module) in arch/arm/kernel/etm.c.
> > That is set up as amba driver.
> 
> Hmm, then again it says this about the scan chains:
> 
>   "SCAN_N ... ARM instruction to select the numbered scan chain used
>   with EXTEST or INTEST. There are six scan chains"
> 
> Which seems like these scan chains are ARM specific.

Or ETM specific even.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Fri, 8 Oct 2010, Tony Lindgren wrote:

> * Nicolas Pitre <nico@fluxnic.net> [101007 18:16]:
> > On Thu, 7 Oct 2010, Tony Lindgren wrote:
> > > 
> > > Can you please pass the read and write functions to the driver
> > > in platform_data? We are already booting kernels with both
> > > ARMv6 and 7 compiled in.
> > 
> > No.  This has nothing to do with platform as this can be determined 
> > within the driver itself.  Would be much better to simply determine 
> > which flavor to use at driver init time and assign two function pointers.
> 
> In the long run some platform init code is needed for powering
> up the JTAG interface and take care of pin multiplexing etc.

Really?  That would be really strange and rather different from all the 
JTAG setups I've seen where the power-up of the interface is done 
externally i.e. controlled by the JTAG dongle.

> Also, isn't DCC (Debug Communications Channel) a JTAG standard?

No.

> At least the following does not say anything about DCC being ARM 
> specific:
> 
> http://en.wikipedia.org/wiki/Joint_Test_Action_Group

So called DCC may be part of an extension built on top of JTAG which is 
not "standardized".  Each vendor implements their own extensions, which 
may or may not include something that can be described as DCC.  The JTAG 
standard concerns itself with only the basic stuff in the full stack.  
You may have a look at OpenOCD to see how wildly different the debugging 
interfaces implemented on top of JTAG are.

Furthermore, the DCC access from within the target are implementation 
specific.  In this case, it is a particular ARM coprocessor access, 
which as the patch shows is not even the same across all ARM versions.  
And that's valid only for those ARM flavors that implement the 
EmbeddedICE or CoreSight.  On XScale, the debug facility built on top of 
the JTAG interface is completely different from the ARM Ltd one, relying 
mostly on software support injected in a special i-cache.  In that case, 
the notion of a DCC would be implemented in software instead of relying 
on a specific hardware register, multiplexed with other messages sent 
over the JTAG interface.

> BTW, we already have ETM (Embedded Trace Module) in arch/arm/kernel/etm.c.
> That is set up as amba driver.

That should be Embedded Trace Macrocell.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Tony Lindgren]

* Nicolas Pitre <nico@fluxnic.net> [101008 14:16]:
> On Fri, 8 Oct 2010, Tony Lindgren wrote:
> 
> > * Nicolas Pitre <nico@fluxnic.net> [101007 18:16]:
> > > On Thu, 7 Oct 2010, Tony Lindgren wrote:
> > > > 
> > > > Can you please pass the read and write functions to the driver
> > > > in platform_data? We are already booting kernels with both
> > > > ARMv6 and 7 compiled in.
> > > 
> > > No.  This has nothing to do with platform as this can be determined 
> > > within the driver itself.  Would be much better to simply determine 
> > > which flavor to use at driver init time and assign two function pointers.
> > 
> > In the long run some platform init code is needed for powering
> > up the JTAG interface and take care of pin multiplexing etc.
> 
> Really?  That would be really strange and rather different from all the 
> JTAG setups I've seen where the power-up of the interface is done 
> externally i.e. controlled by the JTAG dongle.

Well on omaps JTAG is powered by the EMU powerdomain. The docs say
it's software controllable or "automatically on JTAG plug detection".
So yeah JTAG power may be automatic.

But the pin selection is board specific. Some boards may need to use
the JTAG pins for GPIO etc.
 
> > Also, isn't DCC (Debug Communications Channel) a JTAG standard?
> 
> No.
> 
> > At least the following does not say anything about DCC being ARM 
> > specific:
> > 
> > http://en.wikipedia.org/wiki/Joint_Test_Action_Group
> 
> So called DCC may be part of an extension built on top of JTAG which is 
> not "standardized".  Each vendor implements their own extensions, which 
> may or may not include something that can be described as DCC.  The JTAG 
> standard concerns itself with only the basic stuff in the full stack.  
> You may have a look at OpenOCD to see how wildly different the debugging 
> interfaces implemented on top of JTAG are.
> 
> Furthermore, the DCC access from within the target are implementation 
> specific.  In this case, it is a particular ARM coprocessor access, 
> which as the patch shows is not even the same across all ARM versions.  
> And that's valid only for those ARM flavors that implement the 
> EmbeddedICE or CoreSight.  On XScale, the debug facility built on top of 
> the JTAG interface is completely different from the ARM Ltd one, relying 
> mostly on software support injected in a special i-cache.  In that case, 
> the notion of a DCC would be implemented in software instead of relying 
> on a specific hardware register, multiplexed with other messages sent 
> over the JTAG interface.

Thanks for the info. Hmm, there seems to a section for XScale
in arch/arm/kernel/debug.S for elif defined(CONFIG_CPU_XSCALE) for
CONFIG_DEBUG_ICEDCC:

#elif defined(CONFIG_CPU_XSCALE)

		.macro	addruart, rx, tmp
		.endm

		.macro	senduart, rd, rx
		mcr	p14, 0, \rd, c8, c0, 0
		.endm

		.macro	busyuart, rd, rx
1001:
		mrc	p14, 0, \rx, c14, c0, 0
		tst	\rx, #0x10000000
		beq	1001b
		.endm

		.macro	waituart, rd, rx
		mov	\rd, #0x10000000
1001:
		subs	\rd, \rd, #1
		bmi	1002f
		mrc	p14, 0, \rx, c14, c0, 0
		tst	\rx, #0x10000000
		bne	1001b
1002:
		.endm

#else

Is that broken for XScale then?

Regards,

Tony

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Fri, 8 Oct 2010, Tony Lindgren wrote:

> Thanks for the info. Hmm, there seems to a section for XScale
> in arch/arm/kernel/debug.S for elif defined(CONFIG_CPU_XSCALE) for
> CONFIG_DEBUG_ICEDCC:
> 
> #elif defined(CONFIG_CPU_XSCALE)
> 
> 		.macro	addruart, rx, tmp
> 		.endm
> 
> 		.macro	senduart, rd, rx
> 		mcr	p14, 0, \rd, c8, c0, 0
> 		.endm
> 
> 		.macro	busyuart, rd, rx
> 1001:
> 		mrc	p14, 0, \rx, c14, c0, 0
> 		tst	\rx, #0x10000000
> 		beq	1001b
> 		.endm
> 
> 		.macro	waituart, rd, rx
> 		mov	\rd, #0x10000000
> 1001:
> 		subs	\rd, \rd, #1
> 		bmi	1002f
> 		mrc	p14, 0, \rx, c14, c0, 0
> 		tst	\rx, #0x10000000
> 		bne	1001b
> 1002:
> 		.endm
> 
> #else
> 
> Is that broken for XScale then?

Probably not broken, but simplistic.  In a fully debug enabled setup, 
this channel is used to multiplex messages used by the special software 
on the target and the external debugger.  In this case it is used for 
raw string output only.

For example, here's the code for the debugging stubs that gets injected 
into the target by OpenOCD:

http://openocd.git.sourceforge.net/git/gitweb.cgi?p=openocd/openocd;a=blob;f=src/target/xscale/debug_handler.S;h=73f3a9d5e44a87


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Arnd Bergmann]

On Tuesday 05 October 2010, Daniel Walker wrote:
> 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.
> 
> Most of the code was taken from Hyok S. Choi original work, but the
> inline assmebly needed some work and updating. It now supports ARMv7.
> Also the description above is from Hyok also.
> 

Sorry to join in late, but why would you want to make this a "serial"
driver when it really is just a dumb tty.

I think you would be much better off making it a "hvc" driver, where
you just need to provide a read character and write character function
and an optional interrupt handler but otherwise have the common hvc
code take care of polling the hardware and talking to the tty layer.

	Arnd

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-13 at 17:21 +0200, Arnd Bergmann wrote:
> On Tuesday 05 October 2010, Daniel Walker wrote:
> > 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.
> > 
> > Most of the code was taken from Hyok S. Choi original work, but the
> > inline assmebly needed some work and updating. It now supports ARMv7.
> > Also the description above is from Hyok also.
> > 
> 
> Sorry to join in late, but why would you want to make this a "serial"
> driver when it really is just a dumb tty.

The code has been around since 2003 I think, and Hyok wrote (not me)..
So I'm not sure why it's a serial driver. The only thing which I noted
in a prior email is that it uses takes over ttyS optionally, so that
might be part of the reason.

> I think you would be much better off making it a "hvc" driver, where
> you just need to provide a read character and write character function
> and an optional interrupt handler but otherwise have the common hvc
> code take care of polling the hardware and talking to the tty layer.

I don't know what the "hvc" driver is "Hypervisor Virtual Console"
maybe? Can you give any sort of example driver which does what you
suggesting?

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Arnd Bergmann]

On Wednesday 13 October 2010 18:17:03 Daniel Walker wrote:
> > I think you would be much better off making it a "hvc" driver, where
> > you just need to provide a read character and write character function
> > and an optional interrupt handler but otherwise have the common hvc
> > code take care of polling the hardware and talking to the tty layer.
> 
> I don't know what the "hvc" driver is "Hypervisor Virtual Console"
> maybe?

Yes, it originally was used only on hypervisors that had simple
read/write type consoles, but has now turned into a generic facility
that is used by a number of consoles that don't look like classic
serial ports.

> Can you give any sort of example driver which does what you
> suggesting?

Look at drivers/char/hvc_tile.c for the simplest case or
drivers/char/hvc_vio.c for one that uses interrupts.

	Arnd

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-13 at 19:44 +0200, Arnd Bergmann wrote:
> On Wednesday 13 October 2010 18:17:03 Daniel Walker wrote:
> > > I think you would be much better off making it a "hvc" driver, where
> > > you just need to provide a read character and write character function
> > > and an optional interrupt handler but otherwise have the common hvc
> > > code take care of polling the hardware and talking to the tty layer.
> > 
> > I don't know what the "hvc" driver is "Hypervisor Virtual Console"
> > maybe?
> 
> Yes, it originally was used only on hypervisors that had simple
> read/write type consoles, but has now turned into a generic facility
> that is used by a number of consoles that don't look like classic
> serial ports.
> 
> > Can you give any sort of example driver which does what you
> > suggesting?
> 
> Look at drivers/char/hvc_tile.c for the simplest case or
> drivers/char/hvc_vio.c for one that uses interrupts.

I found it independently actually .. It looks like there's at least two
problems. This jtag driver has a status register which flags when RX is
available, and TX is possible. I'm not sure this status register fits
into the model. The other thing is that we have a ttyJ registered for
this driver, and it would be nice to use that over something like ttyHVC
(I'm not sure if that name is correct, just a guess).

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Arnd Bergmann]

On Wednesday 13 October 2010 20:08:35 Daniel Walker wrote:
> On Wed, 2010-10-13 at 19:44 +0200, Arnd Bergmann wrote:
> > On Wednesday 13 October 2010 18:17:03 Daniel Walker wrote:
> > > > I think you would be much better off making it a "hvc" driver, where
> > > > you just need to provide a read character and write character function
> > > > and an optional interrupt handler but otherwise have the common hvc
> > > > code take care of polling the hardware and talking to the tty layer.
> > > 
> > > I don't know what the "hvc" driver is "Hypervisor Virtual Console"
> > > maybe?
> > 
> > Yes, it originally was used only on hypervisors that had simple
> > read/write type consoles, but has now turned into a generic facility
> > that is used by a number of consoles that don't look like classic
> > serial ports.
> > 
> > > Can you give any sort of example driver which does what you
> > > suggesting?
> > 
> > Look at drivers/char/hvc_tile.c for the simplest case or
> > drivers/char/hvc_vio.c for one that uses interrupts.
> 
> I found it independently actually .. It looks like there's at least two
> problems. This jtag driver has a status register which flags when RX is
> available, and TX is possible. I'm not sure this status register fits
> into the model.

I think that is how they all work. The read/write functions simply return
the number of characters transferred, which may be zero if the output
is busy or the input is empty.

> The other thing is that we have a ttyJ registered for
> this driver, and it would be nice to use that over something like ttyHVC
> (I'm not sure if that name is correct, just a guess).

It's hvc0, but I don't see this as a problem because the driver was never
upstream before -- you don't really get to complain about compatibility
with out-of-tree code :-)

Seriously, I don't think you need it, but if you really do, we can probably
find a way to work around this by changing the base hvc driver.

	Arnd

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-13 at 21:45 +0200, Arnd Bergmann wrote:
> On Wednesday 13 October 2010 20:08:35 Daniel Walker wrote:
> > On Wed, 2010-10-13 at 19:44 +0200, Arnd Bergmann wrote:
> > > On Wednesday 13 October 2010 18:17:03 Daniel Walker wrote:
> > > > > I think you would be much better off making it a "hvc" driver, where
> > > > > you just need to provide a read character and write character function
> > > > > and an optional interrupt handler but otherwise have the common hvc
> > > > > code take care of polling the hardware and talking to the tty layer.
> > > > 
> > > > I don't know what the "hvc" driver is "Hypervisor Virtual Console"
> > > > maybe?
> > > 
> > > Yes, it originally was used only on hypervisors that had simple
> > > read/write type consoles, but has now turned into a generic facility
> > > that is used by a number of consoles that don't look like classic
> > > serial ports.
> > > 
> > > > Can you give any sort of example driver which does what you
> > > > suggesting?
> > > 
> > > Look at drivers/char/hvc_tile.c for the simplest case or
> > > drivers/char/hvc_vio.c for one that uses interrupts.
> > 
> > I found it independently actually .. It looks like there's at least two
> > problems. This jtag driver has a status register which flags when RX is
> > available, and TX is possible. I'm not sure this status register fits
> > into the model.
> 
> I think that is how they all work. The read/write functions simply return
> the number of characters transferred, which may be zero if the output
> is busy or the input is empty.
> 
> > The other thing is that we have a ttyJ registered for
> > this driver, and it would be nice to use that over something like ttyHVC
> > (I'm not sure if that name is correct, just a guess).
> 
> It's hvc0, but I don't see this as a problem because the driver was never
> upstream before -- you don't really get to complain about compatibility
> with out-of-tree code :-)

I'm complaining about naming issues, not compatibility.

> Seriously, I don't think you need it, but if you really do, we can probably
> find a way to work around this by changing the base hvc driver.

I think we would have to add something that allows different major/minor
numbers, because something that includes the name "Hypervisor" shouldn't
be used for something general.. /dev/hvcX is also registered for "IBM
iSeries/pSeries virtual console" (according to www.lanana.org) which is
confusing ..

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Arnd Bergmann]

On Wednesday 13 October 2010 21:52:02 Daniel Walker wrote:
> > Seriously, I don't think you need it, but if you really do, we can probably
> > find a way to work around this by changing the base hvc driver.
> 
> I think we would have to add something that allows different major/minor
> numbers, because something that includes the name "Hypervisor" shouldn't
> be used for something general.. /dev/hvcX is also registered for "IBM
> iSeries/pSeries virtual console" (according to www.lanana.org) which is
> confusing ..

That only means we need to update the lanana entry now that the hvc driver
is used on nine different platforms (not including yours) instead of ust
pSeries. Feel free to come up with a backronym for HVC if you're
uncomfortable with the old "hypervisor console" name.

	Arnd

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-13 at 22:10 +0200, Arnd Bergmann wrote:
> On Wednesday 13 October 2010 21:52:02 Daniel Walker wrote:
> > > Seriously, I don't think you need it, but if you really do, we can probably
> > > find a way to work around this by changing the base hvc driver.
> > 
> > I think we would have to add something that allows different major/minor
> > numbers, because something that includes the name "Hypervisor" shouldn't
> > be used for something general.. /dev/hvcX is also registered for "IBM
> > iSeries/pSeries virtual console" (according to www.lanana.org) which is
> > confusing ..
> 
> That only means we need to update the lanana entry now that the hvc driver
> is used on nine different platforms (not including yours) instead of ust
> pSeries. Feel free to come up with a backronym for HVC if you're
> uncomfortable with the old "hypervisor console" name.

If you read back in this thread you'll see how there was a discussion
already on how you shouldn't re-use major/minor numbers for other
purposes .. It looks like "hvc" was essentially overridden , which now
seems like a no-no given that discussion.. This appears to be a
sensitive topic.

It can't hurt anything to make this hvc driver a little more flexible in
this regard ..

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Wed, 13 Oct 2010, Daniel Walker wrote:

> On Wed, 2010-10-13 at 22:10 +0200, Arnd Bergmann wrote:
> > On Wednesday 13 October 2010 21:52:02 Daniel Walker wrote:
> > > > Seriously, I don't think you need it, but if you really do, we can probably
> > > > find a way to work around this by changing the base hvc driver.
> > > 
> > > I think we would have to add something that allows different major/minor
> > > numbers, because something that includes the name "Hypervisor" shouldn't
> > > be used for something general.. /dev/hvcX is also registered for "IBM
> > > iSeries/pSeries virtual console" (according to www.lanana.org) which is
> > > confusing ..
> > 
> > That only means we need to update the lanana entry now that the hvc driver
> > is used on nine different platforms (not including yours) instead of ust
> > pSeries. Feel free to come up with a backronym for HVC if you're
> > uncomfortable with the old "hypervisor console" name.
> 
> If you read back in this thread you'll see how there was a discussion
> already on how you shouldn't re-use major/minor numbers for other
> purposes .. It looks like "hvc" was essentially overridden , which now
> seems like a no-no given that discussion.. This appears to be a
> sensitive topic.

And I violently disagree with this interpretation.

The tty layer was probably the first subsystem to have been implemented 
in Linux.  There are a big deal of legacy with its device naming.  When 
adding new subsystems we don't need to perpetuate this. Maybe that made 
sense 20 years ago, but let's leave ttyS0 alone and use good sense for 
new stuff.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-13 at 16:44 -0400, Nicolas Pitre wrote:
> On Wed, 13 Oct 2010, Daniel Walker wrote:
> 
> > On Wed, 2010-10-13 at 22:10 +0200, Arnd Bergmann wrote:
> > > On Wednesday 13 October 2010 21:52:02 Daniel Walker wrote:
> > > > > Seriously, I don't think you need it, but if you really do, we can probably
> > > > > find a way to work around this by changing the base hvc driver.
> > > > 
> > > > I think we would have to add something that allows different major/minor
> > > > numbers, because something that includes the name "Hypervisor" shouldn't
> > > > be used for something general.. /dev/hvcX is also registered for "IBM
> > > > iSeries/pSeries virtual console" (according to www.lanana.org) which is
> > > > confusing ..
> > > 
> > > That only means we need to update the lanana entry now that the hvc driver
> > > is used on nine different platforms (not including yours) instead of ust
> > > pSeries. Feel free to come up with a backronym for HVC if you're
> > > uncomfortable with the old "hypervisor console" name.
> > 
> > If you read back in this thread you'll see how there was a discussion
> > already on how you shouldn't re-use major/minor numbers for other
> > purposes .. It looks like "hvc" was essentially overridden , which now
> > seems like a no-no given that discussion.. This appears to be a
> > sensitive topic.
> 
> And I violently disagree with this interpretation.
> 
> The tty layer was probably the first subsystem to have been implemented 
> in Linux.  There are a big deal of legacy with its device naming.  When 
> adding new subsystems we don't need to perpetuate this. Maybe that made 
> sense 20 years ago, but let's leave ttyS0 alone and use good sense for 
> new stuff.

Your still confusing me here Nico .. It seems like your suggesting
adding new overrides is not OK , yet your also saying it is ok ?

Are you saying overrides are OK for anything other than ttyS ?

Daniel


-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Wed, 13 Oct 2010, Daniel Walker wrote:

> On Wed, 2010-10-13 at 16:44 -0400, Nicolas Pitre wrote:
> > On Wed, 13 Oct 2010, Daniel Walker wrote:
> > 
> > > On Wed, 2010-10-13 at 22:10 +0200, Arnd Bergmann wrote:
> > > > On Wednesday 13 October 2010 21:52:02 Daniel Walker wrote:
> > > > > > Seriously, I don't think you need it, but if you really do, we can probably
> > > > > > find a way to work around this by changing the base hvc driver.
> > > > > 
> > > > > I think we would have to add something that allows different major/minor
> > > > > numbers, because something that includes the name "Hypervisor" shouldn't
> > > > > be used for something general.. /dev/hvcX is also registered for "IBM
> > > > > iSeries/pSeries virtual console" (according to www.lanana.org) which is
> > > > > confusing ..
> > > > 
> > > > That only means we need to update the lanana entry now that the hvc driver
> > > > is used on nine different platforms (not including yours) instead of ust
> > > > pSeries. Feel free to come up with a backronym for HVC if you're
> > > > uncomfortable with the old "hypervisor console" name.
> > > 
> > > If you read back in this thread you'll see how there was a discussion
> > > already on how you shouldn't re-use major/minor numbers for other
> > > purposes .. It looks like "hvc" was essentially overridden , which now
> > > seems like a no-no given that discussion.. This appears to be a
> > > sensitive topic.
> > 
> > And I violently disagree with this interpretation.
> > 
> > The tty layer was probably the first subsystem to have been implemented 
> > in Linux.  There are a big deal of legacy with its device naming.  When 
> > adding new subsystems we don't need to perpetuate this. Maybe that made 
> > sense 20 years ago, but let's leave ttyS0 alone and use good sense for 
> > new stuff.
> 
> Your still confusing me here Nico .. It seems like your suggesting
> adding new overrides is not OK , yet your also saying it is ok ?

What I'm saying is:

1) The hvc layer is there so it is best if the DCC support uses it.
   Until today I didn't know it existed either, but it offers a higher 
   level of abstraction that the DCC support can use.

2) That level of abstraction already has its device namespace and it
   is perfectly fine to reuse it across different low-level drivers,
   just like SCSI and ATA/SATA disk drives are sharing the same 
   namespace these days.  There used to be a separate namespace for ATA 
   drives but this is now history.

3) All serial drivers could have migrated to a uniform device node 
   namespace when RMK revamped support for serial devices, with dynamic 
   allocation just like everything else.  Unlike for IDE disks, this 
   didn't happen unfortunately because some people couldn't get over a 
   possible device name change (I wonder how they survived the 
   transition to libata).

So if the hvc code has now turned into an abstraction layer that 
supports multiple different devices while still sharing the same device 
namespace I think this is perfect.

So I'm suggesting that the DCC driver be moved to the hvc subsystem.  
And I don't mind if the hvc is reusing the same namespace for different 
devices.  That is what should have happened on a larger scale for all 
serial devices long ago.

If you _insist_ on having a different device name for the DCC port then 
it's probably saner to add that ability to the hvc code than keeping a 
separate serial driver.  But I personally really don't care at that 
point.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Russell King - ARM Linux]

On Wed, Oct 13, 2010 at 06:51:52PM -0400, Nicolas Pitre wrote:
> 3) All serial drivers could have migrated to a uniform device node 
>    namespace when RMK revamped support for serial devices, with dynamic 
>    allocation just like everything else.

Wrong.  There was no dynamic allocation, because the layer above did
not support it.

>  Unlike for IDE disks, this 
>    didn't happen unfortunately because some people couldn't get over a 
>    possible device name change (I wonder how they survived the 
>    transition to libata).

I was not, and still am firmly of the opinion that my decision was
the right one to reject your "approach" to forcefully unifying the
serial devices.  It was more of a hack, introducing multiple
special cases for the old 8250-style ports to make it half sort of
work.

Until the tty layer gets fixed so that it doesn't need to be told
that there will be N tty ports attached to D tty driver at the point
in time when D is registered with the tty subsystem, my opinion on
this isn't going to change.  This is why your patches needed to
special case the 8250 driver - so it could register the right number
of 'ttyS' ports.

And to prove the point that my decision was right, the way support
for ttyS port space has gone, there's been an overwhelming desire
to reduce the number of ttyS ports down to the bare minimum - for
common setups to four at the most.  That'll require yet more special
casing to make your idea work.

The big difference between this and SCSI is that SCSI was built from
day one separate the functional drivers from the backing hardware
drivers - which is great when there's an adhered to standard protocol
which everyone follows.  Again, that's not really the case with serial.
SCSI has also had from day one the ability to dynamically register
and unregister hosts and disks in any order, expanding on the fly with
no problem.  That's certainly not the case with the tty subsystem.

Get over it.  Comparing tty with SCSI is absurdly stupid when they're
vastly different beasts.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Thu, 14 Oct 2010, Russell King - ARM Linux wrote:

> On Wed, Oct 13, 2010 at 06:51:52PM -0400, Nicolas Pitre wrote:
> > 3) All serial drivers could have migrated to a uniform device node 
> >    namespace when RMK revamped support for serial devices, with dynamic 
> >    allocation just like everything else.
> 
> Wrong.  There was no dynamic allocation, because the layer above did
> not support it.
> 
> >  Unlike for IDE disks, this 
> >    didn't happen unfortunately because some people couldn't get over a 
> >    possible device name change (I wonder how they survived the 
> >    transition to libata).
> 
> I was not, and still am firmly of the opinion that my decision was
> the right one to reject your "approach" to forcefully unifying the
> serial devices.  It was more of a hack, introducing multiple
> special cases for the old 8250-style ports to make it half sort of
> work.

I didn't say my patch was right.  But I still think that the general 
idea, which obviously needed more work, was right.  But people didn't 
even agree on the idea.

> Until the tty layer gets fixed so that it doesn't need to be told
> that there will be N tty ports attached to D tty driver at the point
> in time when D is registered with the tty subsystem, my opinion on
> this isn't going to change.

I totally agree.  But that's beside my point.  People even didn't agree 
on the idea that serial port _could_ have dynamically assigned device 
nodes.  In that context there is no point "fixing" the tty layer.  
That's orthogonal to the work you did on unifying serial support as much 
as possible which was excellent.



Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Wed, 13 Oct 2010, Daniel Walker wrote:

> On Wed, 2010-10-13 at 19:44 +0200, Arnd Bergmann wrote:
> > On Wednesday 13 October 2010 18:17:03 Daniel Walker wrote:
> > > > I think you would be much better off making it a "hvc" driver, where
> > > > you just need to provide a read character and write character function
> > > > and an optional interrupt handler but otherwise have the common hvc
> > > > code take care of polling the hardware and talking to the tty layer.
> > > 
> > > I don't know what the "hvc" driver is "Hypervisor Virtual Console"
> > > maybe?
> > 
> > Yes, it originally was used only on hypervisors that had simple
> > read/write type consoles, but has now turned into a generic facility
> > that is used by a number of consoles that don't look like classic
> > serial ports.
> > 
> > > Can you give any sort of example driver which does what you
> > > suggesting?
> > 
> > Look at drivers/char/hvc_tile.c for the simplest case or
> > drivers/char/hvc_vio.c for one that uses interrupts.
> 
> I found it independently actually .. It looks like there's at least two
> problems. This jtag driver has a status register which flags when RX is
> available, and TX is possible. I'm not sure this status register fits
> into the model. The other thing is that we have a ttyJ registered for
> this driver, and it would be nice to use that over something like ttyHVC
> (I'm not sure if that name is correct, just a guess).

Really?  Is there a compelling reason to perpetuate this serial device 
namespace fragmentation nonsense?  Your initial patch even had a config 
option to hijack /dev/ttyS0 because of that.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-13 at 15:55 -0400, Nicolas Pitre wrote:
> > I found it independently actually .. It looks like there's at least two
> > problems. This jtag driver has a status register which flags when RX is
> > available, and TX is possible. I'm not sure this status register fits
> > into the model. The other thing is that we have a ttyJ registered for
> > this driver, and it would be nice to use that over something like ttyHVC
> > (I'm not sure if that name is correct, just a guess).
> 
> Really?  Is there a compelling reason to perpetuate this serial device 
> namespace fragmentation nonsense?  Your initial patch even had a config 
> option to hijack /dev/ttyS0 because of that.

I'm not sure how to interpret what your saying .. Are you saying we
should use /dev/hvcX or shouldn't ? the reason I want to use ttyJ is
because it was assigned specifically for jtags which, to me, makes
things a lot less confusing.

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Nicolas Pitre]

On Wed, 13 Oct 2010, Daniel Walker wrote:

> On Wed, 2010-10-13 at 15:55 -0400, Nicolas Pitre wrote:
> > > I found it independently actually .. It looks like there's at least two
> > > problems. This jtag driver has a status register which flags when RX is
> > > available, and TX is possible. I'm not sure this status register fits
> > > into the model. The other thing is that we have a ttyJ registered for
> > > this driver, and it would be nice to use that over something like ttyHVC
> > > (I'm not sure if that name is correct, just a guess).
> > 
> > Really?  Is there a compelling reason to perpetuate this serial device 
> > namespace fragmentation nonsense?  Your initial patch even had a config 
> > option to hijack /dev/ttyS0 because of that.
> 
> I'm not sure how to interpret what your saying .. Are you saying we
> should use /dev/hvcX or shouldn't ?

Long ago I fought for a uniform namespace for serial ports and alike 
with dynamically assigned names, just like we do for network interfaces, 
for disks, for USB devices, etc. so we'd stop making this hack that 
everybody is doing in their own trees which is to hijack /dev/ttyS0, or 
perpetuate this proliferation of serial/tty device names. This obviously 
didn't happen, for "legacy" reasons (people insisted on having their 
0x2f8 serial port appear as ttyS1 and not ttyS0).

> the reason I want to use ttyJ is because it was assigned specifically 
> for jtags which, to me, makes things a lot less confusing.

Why did the patch have a config option to use ttyS0 then?

Anyway, given that the hvc layer is there and would simplify the DCC 
driver, I think it is a good idea to leverage it instead of duplicating 
and faking tty handling yet again.  Maybe extending the generic hvc code 
to optionally accept alternate device registration could be considered 
instead if you really want a ttyJ device.


Nicolas

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-13 at 16:27 -0400, Nicolas Pitre wrote:
> On Wed, 13 Oct 2010, Daniel Walker wrote:
> 
> > On Wed, 2010-10-13 at 15:55 -0400, Nicolas Pitre wrote:
> > > > I found it independently actually .. It looks like there's at least two
> > > > problems. This jtag driver has a status register which flags when RX is
> > > > available, and TX is possible. I'm not sure this status register fits
> > > > into the model. The other thing is that we have a ttyJ registered for
> > > > this driver, and it would be nice to use that over something like ttyHVC
> > > > (I'm not sure if that name is correct, just a guess).
> > > 
> > > Really?  Is there a compelling reason to perpetuate this serial device 
> > > namespace fragmentation nonsense?  Your initial patch even had a config 
> > > option to hijack /dev/ttyS0 because of that.
> > 
> > I'm not sure how to interpret what your saying .. Are you saying we
> > should use /dev/hvcX or shouldn't ?
> 
> Long ago I fought for a uniform namespace for serial ports and alike 
> with dynamically assigned names, just like we do for network interfaces, 
> for disks, for USB devices, etc. so we'd stop making this hack that 
> everybody is doing in their own trees which is to hijack /dev/ttyS0, or 
> perpetuate this proliferation of serial/tty device names. This obviously 
> didn't happen, for "legacy" reasons (people insisted on having their 
> 0x2f8 serial port appear as ttyS1 and not ttyS0).
> 
> > the reason I want to use ttyJ is because it was assigned specifically 
> > for jtags which, to me, makes things a lot less confusing.
> 
> Why did the patch have a config option to use ttyS0 then?

I don't know exactly why .. Hyok wrote it and I assume there was a good
reason for it, but he's not responding to tell us what it was..

> Anyway, given that the hvc layer is there and would simplify the DCC 
> driver, I think it is a good idea to leverage it instead of duplicating 
> and faking tty handling yet again.  Maybe extending the generic hvc code 
> to optionally accept alternate device registration could be considered 
> instead if you really want a ttyJ device.

That's what I was suggesting to Arng .. We should extend hvc to allow
other major/minor devices.

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] serial: DCC(JTAG) serial and console emulation support

[Daniel Walker]

On Wed, 2010-10-13 at 13:47 -0700, Daniel Walker wrote:

> That's what I was suggesting to Arng .. We should extend hvc to allow
> other major/minor devices.

I mean, Arnd here.. Sorry for the typo..

Daniel
-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.