[PATCH v1 1/4] max3100: added raise_threaded_irq

[PATCH v1 1/4] max3100: added raise_threaded_irq

[Christian Pellegrin]

raise_threaded_irq schedules the execution of an interrupt thread

Signed-off-by: Christian Pellegrin <chripell@fsfe.org>
---
 include/linux/interrupt.h |    3 +++
 kernel/irq/manage.c       |   27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 75f3f00..14c0c13 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -144,6 +144,9 @@ request_threaded_irq(unsigned int irq, irq_handler_t handler,
 static inline void exit_irq_thread(void) { }
 #endif
 
+extern int raise_threaded_irq(unsigned int irq);
+
+
 extern void free_irq(unsigned int, void *);
 
 struct device;
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index eb6078c..a7d21e0 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1088,3 +1088,30 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	return retval;
 }
 EXPORT_SYMBOL(request_threaded_irq);
+
+/**
+ *	raise_threaded_irq - triggers a threded interrupt
+ *	@irq: Interrupt line to trigger
+ */
+int raise_threaded_irq(unsigned int irq)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+	struct irqaction *action;
+
+	if (!desc)
+		return -ENOENT;
+	action = desc->action;
+	if (!action)
+		return -ENOENT;
+	if (unlikely(!action->thread_fn))
+		return -EINVAL;
+	if (likely(!test_bit(IRQTF_DIED,
+			     &action->thread_flags))) {
+		set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
+		wake_up_process(action->thread);
+	} else {
+		return -ECHILD;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(raise_threaded_irq);
-- 
1.5.6.5

[PATCH v1 2/4] max3100: moved to threaded interrupt

[Christian Pellegrin]

The driver was reworked to use threaded interrupts instead of workqueus.
This is a major boost in performance because the former are scheduled as
SCHED_FIFO processes. As a side effect suspend/resume code was greatly
simplified.

Signed-off-by: Christian Pellegrin <chripell@fsfe.org>
---
 drivers/serial/max3100.c |  102 ++++++++++++++-------------------------------
 1 files changed, 32 insertions(+), 70 deletions(-)

diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
index 3c30c56..0c972c6 100644
--- a/drivers/serial/max3100.c
+++ b/drivers/serial/max3100.c
@@ -45,7 +45,9 @@
 #include <linux/serial_core.h>
 #include <linux/serial.h>
 #include <linux/spi/spi.h>
-#include <linux/freezer.h>
+#include <linux/kthread.h>
+#include <linux/interrupt.h>
+#include <linux/bitops.h>
 
 #include <linux/serial_max3100.h>
 
@@ -113,18 +115,15 @@ struct max3100_port {
 
 	int irq;		/* irq assigned to the max3100 */
 
+	/* the workqueue is needed because we cannot schedule
+	   a threaded interrupt during PM
+	 */
+	struct work_struct resume_work;
+
 	int minor;		/* minor number */
 	int crystal;		/* 1 if 3.6864Mhz crystal 0 for 1.8432 */
 	int loopback;		/* 1 if we are in loopback mode */
 
-	/* for handling irqs: need workqueue since we do spi_sync */
-	struct workqueue_struct *workqueue;
-	struct work_struct work;
-	/* set to 1 to make the workhandler exit as soon as possible */
-	int  force_end_work;
-	/* need to know we are suspending to avoid deadlock on workqueue */
-	int suspending;
-
 	/* hook for suspending MAX3100 via dedicated pin */
 	void (*max3100_hw_suspend) (int suspend);
 
@@ -171,13 +170,12 @@ static void max3100_calc_parity(struct max3100_port *s, u16 *c)
 		*c |= max3100_do_parity(s, *c) << 8;
 }
 
-static void max3100_work(struct work_struct *w);
-
-static void max3100_dowork(struct max3100_port *s)
+static void max3100_resume_work(struct work_struct *w)
 {
-	if (!s->force_end_work && !work_pending(&s->work) &&
-	    !freezing(current) && !s->suspending)
-		queue_work(s->workqueue, &s->work);
+	struct max3100_port *s = container_of(w, struct max3100_port,
+					      resume_work);
+
+	raise_threaded_irq(s->irq);
 }
 
 static void max3100_timeout(unsigned long data)
@@ -185,7 +183,7 @@ static void max3100_timeout(unsigned long data)
 	struct max3100_port *s = (struct max3100_port *)data;
 
 	if (s->port.state) {
-		max3100_dowork(s);
+		raise_threaded_irq(s->irq);
 		mod_timer(&s->timer, jiffies + s->poll_time);
 	}
 }
@@ -255,9 +253,9 @@ static int max3100_handlerx(struct max3100_port *s, u16 rx)
 	return ret;
 }
 
-static void max3100_work(struct work_struct *w)
+static irqreturn_t max3100_ist(int irq, void *dev_id)
 {
-	struct max3100_port *s = container_of(w, struct max3100_port, work);
+	struct max3100_port *s = dev_id;
 	int rxchars;
 	u16 tx, rx;
 	int conf, cconf, rts, crts;
@@ -314,23 +312,14 @@ static void max3100_work(struct work_struct *w)
 		if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 			uart_write_wakeup(&s->port);
 
-	} while (!s->force_end_work &&
-		 !freezing(current) &&
+	} while (!kthread_should_stop() &&
 		 ((rx & MAX3100_R) ||
 		  (!uart_circ_empty(xmit) &&
 		   !uart_tx_stopped(&s->port))));
 
 	if (rxchars > 0 && s->port.state->port.tty != NULL)
 		tty_flip_buffer_push(s->port.state->port.tty);
-}
-
-static irqreturn_t max3100_irq(int irqno, void *dev_id)
-{
-	struct max3100_port *s = dev_id;
-
-	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
-	max3100_dowork(s);
 	return IRQ_HANDLED;
 }
 
@@ -353,7 +342,7 @@ static void max3100_start_tx(struct uart_port *port)
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 }
 
 static void max3100_stop_rx(struct uart_port *port)
@@ -369,7 +358,7 @@ static void max3100_stop_rx(struct uart_port *port)
 	s->conf &= ~MAX3100_RM;
 	s->conf_commit = 1;
 	spin_unlock(&s->conf_lock);
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 }
 
 static unsigned int max3100_tx_empty(struct uart_port *port)
@@ -381,7 +370,7 @@ static unsigned int max3100_tx_empty(struct uart_port *port)
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
 	/* may not be truly up-to-date */
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 	return s->tx_empty;
 }
 
@@ -394,7 +383,7 @@ static unsigned int max3100_get_mctrl(struct uart_port *port)
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
 	/* may not be truly up-to-date */
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 	/* always assert DCD and DSR since these lines are not wired */
 	return (s->cts ? TIOCM_CTS : 0) | TIOCM_DSR | TIOCM_CAR;
 }
@@ -414,7 +403,7 @@ static void max3100_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	if (s->rts != rts) {
 		s->rts = rts;
 		s->rts_commit = 1;
-		max3100_dowork(s);
+		raise_threaded_irq(s->irq);
 	}
 	spin_unlock(&s->conf_lock);
 }
@@ -528,7 +517,7 @@ max3100_set_termios(struct uart_port *port, struct ktermios *termios,
 			MAX3100_STATUS_PE | MAX3100_STATUS_FE |
 			MAX3100_STATUS_OE;
 
-	/* we are sending char from a workqueue so enable */
+	/* we are sending char from a threded irq so enable */
 	s->port.state->port.tty->low_latency = 1;
 
 	if (s->poll_time > 0)
@@ -541,7 +530,7 @@ max3100_set_termios(struct uart_port *port, struct ktermios *termios,
 	s->conf_commit = 1;
 	s->parity = parity;
 	spin_unlock(&s->conf_lock);
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 
 	if (UART_ENABLE_MS(&s->port, termios->c_cflag))
 		max3100_enable_ms(&s->port);
@@ -555,19 +544,11 @@ static void max3100_shutdown(struct uart_port *port)
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
-	if (s->suspending)
-		return;
-
-	s->force_end_work = 1;
-
 	if (s->poll_time > 0)
 		del_timer_sync(&s->timer);
 
-	if (s->workqueue) {
-		flush_workqueue(s->workqueue);
-		destroy_workqueue(s->workqueue);
-		s->workqueue = NULL;
-	}
+	flush_work(&s->resume_work);
+
 	if (s->irq)
 		free_irq(s->irq, s);
 
@@ -587,7 +568,6 @@ static int max3100_startup(struct uart_port *port)
 	struct max3100_port *s = container_of(port,
 					      struct max3100_port,
 					      port);
-	char b[12];
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -595,27 +575,15 @@ static int max3100_startup(struct uart_port *port)
 	s->baud = s->crystal ? 230400 : 115200;
 	s->rx_enabled = 1;
 
-	if (s->suspending)
-		return 0;
-
-	s->force_end_work = 0;
 	s->parity = 0;
 	s->rts = 0;
 
-	sprintf(b, "max3100-%d", s->minor);
-	s->workqueue = create_freezeable_workqueue(b);
-	if (!s->workqueue) {
-		dev_warn(&s->spi->dev, "cannot create workqueue\n");
-		return -EBUSY;
-	}
-	INIT_WORK(&s->work, max3100_work);
+	INIT_WORK(&s->resume_work, max3100_resume_work);
 
-	if (request_irq(s->irq, max3100_irq,
+	if (request_threaded_irq(s->irq, NULL, max3100_ist,
 			IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
-		dev_warn(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
+		dev_err(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
 		s->irq = 0;
-		destroy_workqueue(s->workqueue);
-		s->workqueue = NULL;
 		return -EBUSY;
 	}
 
@@ -628,7 +596,7 @@ static int max3100_startup(struct uart_port *port)
 	if (s->max3100_hw_suspend)
 		s->max3100_hw_suspend(0);
 	s->conf_commit = 1;
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 	/* wait for clock to settle */
 	msleep(50);
 
@@ -857,9 +825,6 @@ static int max3100_suspend(struct spi_device *spi, pm_message_t state)
 
 	disable_irq(s->irq);
 
-	s->suspending = 1;
-	uart_suspend_port(&max3100_uart_driver, &s->port);
-
 	if (s->max3100_hw_suspend)
 		s->max3100_hw_suspend(1);
 	else {
@@ -880,14 +845,11 @@ static int max3100_resume(struct spi_device *spi)
 
 	if (s->max3100_hw_suspend)
 		s->max3100_hw_suspend(0);
-	uart_resume_port(&max3100_uart_driver, &s->port);
-	s->suspending = 0;
 
 	enable_irq(s->irq);
-
+	/* must reconfigure if power was cut-off the chip during suspend */
 	s->conf_commit = 1;
-	if (s->workqueue)
-		max3100_dowork(s);
+	schedule_work(&s->resume_work);
 
 	return 0;
 }
-- 
1.5.6.5

[PATCH v1 3/4] max3100: adds console support for MAX3100

[Christian Pellegrin]

This patch adds console support for the MAX3100 UART
(console=ttyMAX0,11500). The SPI subsystem and an
suitable SPI master driver have to be compiled in the kernel.

Signed-off-by: Christian Pellegrin <chripell@fsfe.org>
---
 drivers/serial/Kconfig         |    7 ++
 drivers/serial/max3100.c       |  217 +++++++++++++++++++++++++++++++++++++---
 include/linux/serial_max3100.h |    4 +
 3 files changed, 212 insertions(+), 16 deletions(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 9ff47db..acd758c 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -547,6 +547,13 @@ config SERIAL_MAX3100
 	help
 	  MAX3100 chip support
 
+config SERIAL_MAX3100_CONSOLE
+	bool "Support console on MAX3100"
+	depends on SERIAL_MAX3100=y
+	select SERIAL_CORE_CONSOLE
+	help
+	  Console on MAX3100
+
 config SERIAL_DZ
 	bool "DECstation DZ serial driver"
 	depends on MACH_DECSTATION && 32BIT
diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
index 0c972c6..ec3e13a 100644
--- a/drivers/serial/max3100.c
+++ b/drivers/serial/max3100.c
@@ -48,6 +48,7 @@
 #include <linux/kthread.h>
 #include <linux/interrupt.h>
 #include <linux/bitops.h>
+#include <linux/console.h>
 
 #include <linux/serial_max3100.h>
 
@@ -131,6 +132,22 @@ struct max3100_port {
 	int poll_time;
 	/* and its timer */
 	struct timer_list	timer;
+
+	int console_flags;
+	/* is this port a console? */
+#define MAX3100_IS_CONSOLE   (1 << 0)
+	/* are we in suspend/resume? */
+#define MAX3100_SUSPENDING   (1 << 1)
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+	/* console buffer */
+#define CONSOLE_BUF_SIZE 4096
+	char *console_buf;
+	int console_head, console_tail;
+	/* delayed console work because we may have to sleep */
+	struct work_struct console_work;
+	/* char tx timeout */
+	int console_tout;
+#endif
 };
 
 static struct max3100_port *max3100s[MAX_MAX3100]; /* the chips */
@@ -175,7 +192,9 @@ static void max3100_resume_work(struct work_struct *w)
 	struct max3100_port *s = container_of(w, struct max3100_port,
 					      resume_work);
 
-	raise_threaded_irq(s->irq);
+	if (s->irq)
+		raise_threaded_irq(s->irq);
+	s->console_flags &= ~MAX3100_SUSPENDING;
 }
 
 static void max3100_timeout(unsigned long data)
@@ -552,14 +571,16 @@ static void max3100_shutdown(struct uart_port *port)
 	if (s->irq)
 		free_irq(s->irq, s);
 
-	/* set shutdown mode to save power */
-	if (s->max3100_hw_suspend)
-		s->max3100_hw_suspend(1);
-	else  {
-		u16 tx, rx;
+	if (!(s->console_flags & MAX3100_IS_CONSOLE)) {
+		/* set shutdown mode to save power */
+		if (s->max3100_hw_suspend) {
+			s->max3100_hw_suspend(1);
+		} else {
+			u16 tx, rx;
 
-		tx = MAX3100_WC | MAX3100_SHDN;
-		max3100_sr(s, tx, &rx);
+			tx = MAX3100_WC | MAX3100_SHDN;
+			max3100_sr(s, tx, &rx);
+		}
 	}
 }
 
@@ -578,8 +599,6 @@ static int max3100_startup(struct uart_port *port)
 	s->parity = 0;
 	s->rts = 0;
 
-	INIT_WORK(&s->resume_work, max3100_resume_work);
-
 	if (request_threaded_irq(s->irq, NULL, max3100_ist,
 			IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
 		dev_err(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
@@ -699,6 +718,147 @@ static struct uart_ops max3100_ops = {
 	.verify_port	= max3100_verify_port,
 };
 
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+
+static void max3100_console_work(struct work_struct *w)
+{
+	struct max3100_port *s = container_of(w, struct max3100_port,
+					      console_work);
+	unsigned long start;
+	u16 tx, rx;
+
+	while (s->console_head != s->console_tail &&
+	       (s->console_flags & MAX3100_SUSPENDING) == 0) {
+		start = jiffies;
+		do {
+			tx = MAX3100_RC;
+			max3100_sr(s, tx, &rx);
+		} while ((rx & MAX3100_T) == 0 &&
+			 !time_after(jiffies, start + s->console_tout));
+		tx = s->console_buf[s->console_tail];
+		max3100_calc_parity(s, &tx);
+		tx |= MAX3100_WD | MAX3100_RTS;
+		max3100_sr(s, tx, &rx);
+		s->console_tail = (s->console_tail + 1) % CONSOLE_BUF_SIZE;
+	}
+}
+
+static void max3100_console_putchar(struct uart_port *port, int ch)
+{
+	struct max3100_port *s = to_max3100_port(port);
+	int next = (s->console_head + 1) % CONSOLE_BUF_SIZE;
+
+	if (next != s->console_tail) {
+		s->console_buf[next] = ch;
+		s->console_head = next;
+	}
+}
+
+static void max3100_console_write(struct console *co,
+				  const char *str, unsigned int count)
+{
+	struct max3100_port *s = max3100s[co->index];;
+
+	uart_console_write(&s->port, str, count, max3100_console_putchar);
+	schedule_work(&s->console_work);
+}
+
+static int max3100_console_setup(struct console *co, char *options)
+{
+	struct max3100_port *s;
+	int baud = 115200;
+	int bits = 8;
+	int parity = 'n';
+	int flow = 'n';
+	int ret;
+	u16 tx, rx;
+
+	if (co->index == -1 || co->index >= MAX_MAX3100)
+		co->index = 0;
+	s = max3100s[co->index];
+	if (!s)
+		return -ENOENT;
+
+	s->console_buf = kmalloc(CONSOLE_BUF_SIZE, GFP_KERNEL);
+	if (!s->console_buf)
+		return -ENOMEM;
+	INIT_WORK(&s->console_work, max3100_console_work);
+	s->console_flags |= MAX3100_IS_CONSOLE;
+
+	if (options)
+		uart_parse_options(options, &baud, &parity, &bits, &flow);
+	ret = uart_set_options(&s->port, co, baud, parity, bits, flow);
+
+	tx = 0;
+	switch (baud) {
+	case 300:
+		tx = 15;
+		break;
+	case 600:
+		tx = 14 + s->crystal;
+		break;
+	case 1200:
+		tx = 13 + s->crystal;
+		break;
+	case 2400:
+		tx = 12 + s->crystal;
+		break;
+	case 4800:
+		tx = 11 + s->crystal;
+		break;
+	case 9600:
+		tx = 10 + s->crystal;
+		break;
+	case 19200:
+		tx = 9 + s->crystal;
+		break;
+	case 38400:
+		tx = 8 + s->crystal;
+		break;
+	case 57600:
+		tx = 1 + s->crystal;
+		break;
+	case 115200:
+		tx = 0 + s->crystal;
+		break;
+	case 230400:
+		tx = 0;
+		break;
+	}
+	if (bits == 7) {
+		tx |= MAX3100_L;
+		s->parity |= MAX3100_7BIT;
+	}
+	if (parity == 'o' || parity == 'e') {
+		tx |= MAX3100_PE;
+		s->parity |= MAX3100_PARITY_ON;
+	}
+	if (parity == 'o')
+		s->parity |= MAX3100_PARITY_ODD;
+	s->console_tout = 1 + (20 * HZ) / baud; /* jiffies to send 20 bits */
+
+	tx |= MAX3100_WC;
+	max3100_sr(s, tx, &rx);
+
+	/* wait for oscilator to stabilize. Data sheet says 25ms,
+	 * apply "multiply by two" engineer's rule */
+	msleep(50);
+	return ret;
+}
+
+static struct uart_driver max3100_uart_driver;
+static struct console max3100_console = {
+	.name		= "ttyMAX",
+	.write		= max3100_console_write,
+	.device		= uart_console_device,
+	.setup		= max3100_console_setup,
+	.flags		= CON_PRINTBUFFER,
+	.index		= -1,
+	.data		= &max3100_uart_driver,
+};
+static int max3100_console_registered;
+#endif
+
 static struct uart_driver max3100_uart_driver = {
 	.owner          = THIS_MODULE,
 	.driver_name    = "ttyMAX",
@@ -706,6 +866,9 @@ static struct uart_driver max3100_uart_driver = {
 	.major          = MAX3100_MAJOR,
 	.minor          = MAX3100_MINOR,
 	.nr             = MAX_MAX3100,
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+	.cons           = &max3100_console,
+#endif
 };
 static int uart_driver_registered;
 
@@ -768,18 +931,27 @@ static int __devinit max3100_probe(struct spi_device *spi)
 	max3100s[i]->port.line = i;
 	max3100s[i]->port.type = PORT_MAX3100;
 	max3100s[i]->port.dev = &spi->dev;
+	INIT_WORK(&max3100s[i]->resume_work, max3100_resume_work);
 	retval = uart_add_one_port(&max3100_uart_driver, &max3100s[i]->port);
 	if (retval < 0)
 		dev_warn(&spi->dev,
 			 "uart_add_one_port failed for line %d with error %d\n",
 			 i, retval);
 
-	/* set shutdown mode to save power. Will be woken-up on open */
-	if (max3100s[i]->max3100_hw_suspend)
-		max3100s[i]->max3100_hw_suspend(1);
-	else {
-		tx = MAX3100_WC | MAX3100_SHDN;
-		max3100_sr(max3100s[i], tx, &rx);
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+	if (pdata->console && !max3100_console_registered) {
+		register_console(&max3100_console);
+		max3100_console_registered = 1;
+	}
+#endif
+	if (!(max3100s[i]->console_flags & MAX3100_IS_CONSOLE)) {
+		/* set shutdown mode to save power. Will be woken-up on open */
+		if (max3100s[i]->max3100_hw_suspend) {
+			max3100s[i]->max3100_hw_suspend(1);
+		} else {
+			tx = MAX3100_WC | MAX3100_SHDN;
+			max3100_sr(max3100s[i], tx, &rx);
+		}
 	}
 	mutex_unlock(&max3100s_lock);
 	return 0;
@@ -792,12 +964,24 @@ static int __devexit max3100_remove(struct spi_device *spi)
 
 	mutex_lock(&max3100s_lock);
 
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+	if (max3100_console_registered) {
+		unregister_console(&max3100_console);
+		max3100_console_registered = 0;
+	}
+#endif
 	/* find out the index for the chip we are removing */
 	for (i = 0; i < MAX_MAX3100; i++)
 		if (max3100s[i] == s)
 			break;
 
 	dev_dbg(&spi->dev, "%s: removing port %d\n", __func__, i);
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+	if (s->console_buf) {
+		flush_work(&s->console_work);
+		kfree(s->console_buf);
+	}
+#endif
 	uart_remove_one_port(&max3100_uart_driver, &max3100s[i]->port);
 	kfree(max3100s[i]);
 	max3100s[i] = NULL;
@@ -823,6 +1007,7 @@ static int max3100_suspend(struct spi_device *spi, pm_message_t state)
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
+	s->console_flags |= MAX3100_SUSPENDING;
 	disable_irq(s->irq);
 
 	if (s->max3100_hw_suspend)
diff --git a/include/linux/serial_max3100.h b/include/linux/serial_max3100.h
index 4976bef..71fb862 100644
--- a/include/linux/serial_max3100.h
+++ b/include/linux/serial_max3100.h
@@ -21,6 +21,9 @@
  *                       called on suspend and resume to activate it.
  * @poll_time:           poll time for CTS signal in ms, 0 disables (so no hw
  *                       flow ctrl is possible but you have less CPU usage)
+ * @console:             1 if this MAX3100 is the last one on the system
+ *                       (in the order of spi_board_info enumeration) that
+ *                       can act as a console.
  *
  * You should use this structure in your machine description to specify
  * how the MAX3100 is connected. Example:
@@ -47,6 +50,7 @@ struct plat_max3100 {
 	int crystal;
 	void (*max3100_hw_suspend) (int suspend);
 	int poll_time;
+	int console;
 };
 
 #endif
-- 
1.5.6.5

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[Feng Tang]

Hi,

I modified the code a little and run it on our HW platform, it really show
some sigh of life: it can boots to console (the print format is not so good),
I can input command and it execute correctly, but very slow, I type 3
characters and it takes about 2 seconds to echo back on screen and start the
execution, and after about 1 minute, the console hang there and input stopped
to work.

I also have some comments inline.

Thanks,
Feng


On Tue, 23 Mar 2010 18:29:30 +0800
Christian Pellegrin <chripell@fsfe.org> wrote:

> This patch adds console support for the MAX3100 UART
> (console=ttyMAX0,11500). The SPI subsystem and an

115200?

>  
> +#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
> +
> +static void max3100_console_work(struct work_struct *w)
> +{
> +	struct max3100_port *s = container_of(w, struct max3100_port,
> +					      console_work);
> +	unsigned long start;
> +	u16 tx, rx;
> +
> +	while (s->console_head != s->console_tail &&
> +	       (s->console_flags & MAX3100_SUSPENDING) == 0) {
> +		start = jiffies;
> +		do {
> +			tx = MAX3100_RC;
> +			max3100_sr(s, tx, &rx);
> +		} while ((rx & MAX3100_T) == 0 &&
> +			 !time_after(jiffies, start +
> s->console_tout));
> +		tx = s->console_buf[s->console_tail];
> +		max3100_calc_parity(s, &tx);
> +		tx |= MAX3100_WD | MAX3100_RTS;

Does this imply to have to work with HW flow control? on my platform
I have to remove the RTS bit to make it work.

> +		max3100_sr(s, tx, &rx);

It doesn't handle received characters here? If the console is printing out
a bulk of message while user input some command, the command may be ignored.
Myself have met the same problem in our driver.


> +		s->console_tail = (s->console_tail + 1) %
> CONSOLE_BUF_SIZE;
> +	}
> +}
> +
> +static void max3100_console_putchar(struct uart_port *port, int ch)
> +{
> +	struct max3100_port *s = to_max3100_port(port);
> +	int next = (s->console_head + 1) % CONSOLE_BUF_SIZE;
> +
> +	if (next != s->console_tail) {
> +		s->console_buf[next] = ch;
> +		s->console_head = next;
> +	}

Also I saw max3100_sr() uses cpu_to_be16() and be16_to_cpu(), is it really
necessary, our platform is little-endian(x86), and I have to disable them
to make the code work. Is your test platform big-endian?

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[christian pellegrin]

On Mon, Mar 29, 2010 at 4:48 AM, Feng Tang <feng.tang@intel.com> wrote:
> Hi,

Hi,

>
> I modified the code a little and run it on our HW platform, it really show
> some sigh of life: it can boots to console (the print format is not so good),
> I can input command and it execute correctly, but very slow, I type 3
> characters and it takes about 2 seconds to echo back on screen and start the
> execution, and after about 1 minute, the console hang there and input stopped
> to work.

never seen such a behavior. Which platform are you using? Which SPI
driver? Do you have a low level printk (printascii) that puts output
somewhere else so I can send you a patch with some debugging output?
Can you log in some other way (like via network) and see if the CPU
load is at 100% for some reason?

>> This patch adds console support for the MAX3100 UART
>> (console=ttyMAX0,11500). The SPI subsystem and an
>
> 115200?
>

ack

> Does this imply to have to work with HW flow control? on my platform
> I have to remove the RTS bit to make it work.
>

no, I put RTS on  because it looks like a good default. I can make it
configurable. I just noticed on the data sheet that RTS is actually
inverted so a more sensible default would be to put it off. For
testing you should have flow control set to none on the machine you
are using as a terminal emulator.

>> +             max3100_sr(s, tx, &rx);
>
> It doesn't handle received characters here? If the console is printing out
> a bulk of message while user input some command, the command may be ignored.
> Myself have met the same problem in our driver.
>

yes but I think it's quite difficult to solve this problem in every
case. Console output is massively used only on boot when the user is
not supposed to type a lot.

>> +     if (next != s->console_tail) {
>> +             s->console_buf[next] = ch;
>> +             s->console_head = next;
>> +     }
>
> Also I saw max3100_sr() uses cpu_to_be16() and be16_to_cpu(), is it really
> necessary, our platform is little-endian(x86), and I have to disable them
> to make the code work. Is your test platform big-endian?
>

Have you configured your SPI controller as LSB first somehow, haven't
you? BTW my platform is a quite usual ARM9 S3C2440 which is little
endian.


-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[Feng Tang]

On Mon, 29 Mar 2010 14:11:15 +0800
christian pellegrin <chripell@fsfe.org> wrote:

> > I modified the code a little and run it on our HW platform, it
> > really show some sigh of life: it can boots to console (the print
> > format is not so good), I can input command and it execute
> > correctly, but very slow, I type 3 characters and it takes about 2
> > seconds to echo back on screen and start the execution, and after
> > about 1 minute, the console hang there and input stopped to work.
> 
> never seen such a behavior. Which platform are you using? Which SPI
> driver? Do you have a low level printk (printascii) that puts output
> somewhere else so I can send you a patch with some debugging output?
> Can you log in some other way (like via network) and see if the CPU
> load is at 100% for some reason?

Hi,

Our platform is Intel Moorestown platform, and use a spi controller core
from Designware (drivers/spi/dw_*.c). I know the problem may probably be
caused by my setting, but the dw_spi driver works fine with our own
3110 driver.

For debug method, sadly I don't get another output port yet, but if you
have some debug patch, that's great, it will help when I find another debug
output than max3110.

> 
> >> +             max3100_sr(s, tx, &rx);
> >
> > It doesn't handle received characters here? If the console is
> > printing out a bulk of message while user input some command, the
> > command may be ignored. Myself have met the same problem in our
> > driver.
> >
> 
> yes but I think it's quite difficult to solve this problem in every
> case. Console output is massively used only on boot when the user is
> not supposed to type a lot.

It's difficult but not impossible, actually our driver checks every word
read back and handle it if it contains a valid data

> 
> >> +     if (next != s->console_tail) {
> >> +             s->console_buf[next] = ch;
> >> +             s->console_head = next;
> >> +     }
> >
> > Also I saw max3100_sr() uses cpu_to_be16() and be16_to_cpu(), is it
> > really necessary, our platform is little-endian(x86), and I have to
> > disable them to make the code work. Is your test platform
> > big-endian?
> >
> 
> Have you configured your SPI controller as LSB first somehow, haven't
> you? BTW my platform is a quite usual ARM9 S3C2440 which is little
> endian.
>
yeah, you hit the point that our spi controller is LSB naturally (not
configured to), here may need a check for whether to do a swap

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

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[christian pellegrin]

On Mon, Mar 29, 2010 at 9:06 AM, Feng Tang <feng.tang@intel.com> wrote:

> Our platform is Intel Moorestown platform, and use a spi controller core
> from Designware (drivers/spi/dw_*.c). I know the problem may probably be
> caused by my setting, but the dw_spi driver works fine with our own
> 3110 driver.

I had a look at the dw_spi driver. The spi_transfer path queues some
work to a worqueue that itself schedules a tasklet. I don't think this
is good for latency, I won't bet that such an architecture could
deliver good performance. Now I see why you needed to do only big fat
SPI transfers. Anyway this doesn't justify the 2 seconds delay between
chars coming in and going out through the max31x0 you are seeing. I
will try to analyze what's going on. BTW is only input slow or output
is sluggish too? The initial messages from the console are coming out
fast? If you disable the MAX3110 for console but you use just as a
normal terminal (set console=/dev/null in the kernel command line and
add getty 115200 /dev/ttyMAX0 in iniitab) is the interaction with the
system fine? Thanks for helping sorting out this.

>>
>> yes but I think it's quite difficult to solve this problem in every
>> case. Console output is massively used only on boot when the user is
>> not supposed to type a lot.
>
> It's difficult but not impossible, actually our driver checks every word
> read back and handle it if it contains a valid data
>

Of course it is possible, I just wanted to keep the max3100 a small
clean driver. Unfortunately console and serial drivers are two
different beings in the Linux kernel, but the max3100 implements the
tx-rx in one indivisible instruction (that is slow compared to
registers IO and has to be called in an preemptible contex for added
fun).  To implement what you are saying we need:

1) the console code has to check if the serial port associated to the
same physical max3100 is up (console driver start their life much
before serial ones).

2) if yes send data to the tty associated to the serial driver.
Locking is needed here.

I will implement this ASAP.

>> Have you configured your SPI controller as LSB first somehow, haven't
>> you? BTW my platform is a quite usual ARM9 S3C2440 which is little
>> endian.
>>
> yeah, you hit the point that our spi controller is LSB naturally (not
> configured to), here may need a check for whether to do a swap
>

ok, I think the dw_spi driver has to be fixed.


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

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[Feng Tang]

On Mon, 29 Mar 2010 20:55:51 +0800
christian pellegrin <chripell@fsfe.org> wrote:

> On Mon, Mar 29, 2010 at 9:06 AM, Feng Tang <feng.tang@intel.com>
> wrote:
> 
> > Our platform is Intel Moorestown platform, and use a spi controller
> > core from Designware (drivers/spi/dw_*.c). I know the problem may
> > probably be caused by my setting, but the dw_spi driver works fine
> > with our own 3110 driver.
> 
> I had a look at the dw_spi driver. The spi_transfer path queues some
> work to a worqueue that itself schedules a tasklet. I don't think this
> is good for latency, I won't bet that such an architecture could
> deliver good performance. Now I see why you needed to do only big fat
> SPI transfers. Anyway this doesn't justify the 2 seconds delay between
Hi

DW controller driver don't need max3110 driver to use big spi transfer,
the early version of our max3110 is also one word per spi transfer mode,
and the 2 drivers work fine, though the rx performance is not good (copy
& paste a bulk message to console).

Let me use some example to explain why I use big spi transfer for 3110:

When the HW works at 230400 bps, when we copy bulk data to the console,
max3110 will probably receive about 20K characters, so the time for
handling each char is 50us. If you use one char per spi transfer, it
will have to execute 20K spi_transfer in one second, and each need be
done in 50us, in this 50us you'll have to deal with controller IRQ 
handler + spi system overhead + tty receiving overhead. Is it a little
over-killing to use one char per spi transfer? while the HW does
have a 8 words RX FIFO 

> chars coming in and going out through the max31x0 you are seeing. I
> will try to analyze what's going on. BTW is only input slow or output
> is sluggish too? The initial messages from the console are coming out
> fast? If you disable the MAX3110 for console but you use just as a
> normal terminal (set console=/dev/null in the kernel command line and
> add getty 115200 /dev/ttyMAX0 in iniitab) is the interaction with the
> system fine? Thanks for helping sorting out this.

The output is not so slow as input, if we set the console=/dev/ttyMAX0,
the output is sluggish, looks like below, but when enter command line
console the output is smooth, while the input is very slow.

--------------------------------------------------------------
   kernel print log of max3100 over Moorestown platform
--------------------------------------------------------------

    0.000000] Moorestown CPU Lincroft identified
[[    ..000000  iiuu  eessoo  ....44rr11((eeggffnn--77  ggccvvrriinn444411((bbnnuu44441144bbnnuu))))
[[    ..000000  BBOO--8800  0000000000001100--0000000000008800  uuaall))

                                                                            00000000]]  IISSee22::00
00000000110000 - 000000000f2000000 (usabl))
                                           [     .0000000  BIOS-e822:: 000000000220000    0000000110
000000((eserved)
[    0.000000]  BIISSe820:: 00000000ee00000 --000000000fec01000 (reseree))

                                                                              00000000]]  IISSee200
[[    ..000000  BBOO--8800  00000000ff000000--0000000000000000  rrssrree))

                                                                              00000000]]NNttcc::NN
[[    ..000000  8800uuddtt  aagg::0000000000000000    0000000000000000((ssbbee  ==  rrssrree))

                                                                                                  00
000000]]ee22  eeooeerrnnee  0000000000aa0000--0000000000110000  uuaall))

                                                                            00000000]]llss__ff    xx
2200mmxxaacc__ff    xx000000

                                00000000]]MMRR  eeaall  yyee  nnaahhbbee

                                                                            00000000]]MMRR  iiee  aa
[[    ..000000    0000--FFFF  rrtt--aakk

[[    ..000000    0000--FFFF  rrtt--aakk    00000000]]  AA0000BBFFFFuuccccaall

                                            00000000]]MMRR  aaiibbeerrnnee  nnbbee::




> 1) the console code has to check if the serial port associated to the
> same physical max3100 is up (console driver start their life much
> before serial ones).
> 
> 2) if yes send data to the tty associated to the serial driver.
> Locking is needed here.
> 
> I will implement this ASAP.

cool!

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[christian pellegrin]

On Tue, Mar 30, 2010 at 4:14 AM, Feng Tang <feng.tang@intel.com> wrote:
> On Mon, 29 Mar 2010 20:55:51 +0800

> DW controller driver don't need max3110 driver to use big spi transfer,
> the early version of our max3110 is also one word per spi transfer mode,
> and the 2 drivers work fine, though the rx performance is not good (copy
> & paste a bulk message to console).

What I'm saying is that the reason of bad performance is that the
underlying SPI driver has bad performance. A SPI master driver should
do as little work as possible. For small transfers (2 bytes == 16
bits) at 5MHz (== 3.2 us) it's much better to spin in spi_transfer
waiting for SPI done than scheduling anything or setting up a DMA
transfer. Especially if you do *2* task schedule (spi_transfer queues
a workqueue that schedules a tasklet) be prepared for *big* latency. I
hope other people can comment on this if I'm saying something wrong.

>
> When the HW works at 230400 bps, when we copy bulk data to the console,
> max3110 will probably receive about 20K characters, so the time for
> handling each char is 50us. If you use one char per spi transfer, it
> will have to execute 20K spi_transfer in one second, and each need be
> done in 50us, in this 50us you'll have to deal with controller IRQ
> handler + spi system overhead + tty receiving overhead. Is it a little
> over-killing to use one char per spi transfer? while the HW does
> have a 8 words RX FIFO

this is too hackish, max3100 wasn't conceived this way. Let me point
some problems of such an approach:

1) latency on rx chars becomes very high because we can process
incoming transfers only after a full 8 byte (or whatever the spi
transfer dimension is). For a 9600 this is too much.

2) even worse is that we can do flow control decision only on such boundary.

3) this is not reliable: think of what happens if the actual SPI
transfer speed we get will be slower that the one we requested. We
won't be emptying the RX buffer fastly enough even if could.

4) for low speeds we are going to monopolize the SPI bus for ages.

So I'm rather convinced that the SPI transfer has to happen as soon as
possible at a SPI device driver level without any guess on how the
data will be clocked out. I suggest you to improve the dw_spi driver
for better performance.

> [[    ..000000  iiuu  eessoo  ....44rr11((eeggffnn--77  ggccvvrriinn444411((bbnnuu44441144bbnnuu))))
> [[    ..000000  BBOO--8800  0000000000001100--0000000000008800  uuaall))

huh, here I'm seeing another kind of problem: the same char is
repeated two times (for example BBOO is written instead of BIOS). I'm
quite sure (but I will triple check shortly) the the console driver
doesn't do this (it's quite an easy loop in max3100_console_work, I
will check wit a printascii to another serial port what is sent). So I
guess the problem can be in the SPI master driver: is it correctly
handling the CS changes? Isn't it having problems with DMA for example
(I always found DMA for small data transfer (such 2 bytes in this
case) problematic (for example many platforms just do it on a 4 byte
boundary))? Have you tested it with other devices?

-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[Feng Tang]

On Tue, 30 Mar 2010 14:49:57 +0800
christian pellegrin <chripell@fsfe.org> wrote:

> On Tue, Mar 30, 2010 at 4:14 AM, Feng Tang <feng.tang@intel.com>
> wrote:
> > On Mon, 29 Mar 2010 20:55:51 +0800
> >
> > When the HW works at 230400 bps, when we copy bulk data to the
> > console, max3110 will probably receive about 20K characters, so the
> > time for handling each char is 50us. If you use one char per spi
> > transfer, it will have to execute 20K spi_transfer in one second,
> > and each need be done in 50us, in this 50us you'll have to deal
> > with controller IRQ handler + spi system overhead + tty receiving
> > overhead. Is it a little over-killing to use one char per spi
> > transfer? while the HW does have a 8 words RX FIFO
> 
> this is too hackish, max3100 wasn't conceived this way. Let me point
> some problems of such an approach:
> 
> 1) latency on rx chars becomes very high because we can process
> incoming transfers only after a full 8 byte (or whatever the spi
> transfer dimension is). For a 9600 this is too much.
> 
> 2) even worse is that we can do flow control decision only on such
> boundary.
> 
> 3) this is not reliable: think of what happens if the actual SPI
> transfer speed we get will be slower that the one we requested. We
> won't be emptying the RX buffer fastly enough even if could.
> 
> 4) for low speeds we are going to monopolize the SPI bus for ages.

No words in my last email suggested to use dynamic SPI clock speed,
I admited that method is brutal and not mature when my driver was
reviewed. In early version of our 3110 driver, we don't use dynamic
clock speed, but the maxim clock supports by 3110, while we still use 
8 words buffer read for RX.

Let me cut my question clear: why not use the 8 words RX HW fifo? For
bulk RX in 230400 case, is 50us enough to handle controller IRQ +
spi subsystem overhead + tty receive overhead for one char per spi
transfer model?

> 
> So I'm rather convinced that the SPI transfer has to happen as soon as
> possible at a SPI device driver level without any guess on how the
> data will be clocked out. I suggest you to improve the dw_spi driver
> for better performance.
Will consider that, thanks

> 
> > [[    ..000000  iiuu  eessoo  ....44rr11((eeggffnn--77
> >  ggccvvrriinn444411((bbnnuu44441144bbnnuu)))) [[    ..000000
> >  BBOO--8800  0000000000001100--0000000000008800  uuaall))
> 
> huh, here I'm seeing another kind of problem: the same char is
> repeated two times (for example BBOO is written instead of BIOS). I'm
> quite sure (but I will triple check shortly) the the console driver
> doesn't do this (it's quite an easy loop in max3100_console_work, I
> will check wit a printascii to another serial port what is sent). So I
> guess the problem can be in the SPI master driver: is it correctly
> handling the CS changes? Isn't it having problems with DMA for example
> (I always found DMA for small data transfer (such 2 bytes in this
> case) problematic (for example many platforms just do it on a 4 byte
> boundary))? Have you tested it with other devices?
> 
Yes, the controller HW has a register to chose slave devices and thus
the HW CS handling is transparent to software. The controller supports several
slave devices connecting to it simultaneously, one is a 3G modem whose
throughput is about several Mega bits. The current in-tree controller driver
lacks DMA part, which is rejected as the related DMA controller driver never
show up publicly.


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

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[christian pellegrin]

On Tue, Mar 30, 2010 at 9:19 AM, Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> No words in my last email suggested to use dynamic SPI clock speed,
> I admited that method is brutal and not mature when my driver was
> reviewed. In early version of our 3110 driver, we don't use dynamic
> clock speed, but the maxim clock supports by 3110, while we still use
> 8 words buffer read for RX.
>
> Let me cut my question clear: why not use the 8 words RX HW fifo? For
> bulk RX in 230400 case, is 50us enough to handle controller IRQ +
> spi subsystem overhead + tty receive overhead for one char per spi
> transfer model?
>

For TX it's impossible. Before sending a char we have to check if T
bit says that the TX buffer is empty. Otherwise if SPI bus speed >
UART baud rate we will loose TX chars for sure. For RX maybe it might
be worth to always do 8 byte transfers and the look at R bit of
transfer i to see if we have a valid char in (i+1) instead of doing
single transfers. But here again we are fixing the insane latency per
start of transfer of the underlying SPI master controller. And let me
say that in the actual driver the 8 byte RX fifo *is* used: the loop
in max3100_ist drains RX buffer at a speed greater than the chars
coming (also at 115200). The 8-bytes RX buffer fills as a consequence
of delays in interrupt thread being awaken.

The tests I did with zmodem show that in the direction max3100 <- PC
has very good efficiency compared to max3100 -> PC. I guess the reason
is that the tx buffer is just 1 byte deep and we don't have any other
way to handle the T bit other than the do loop in max3100_ist. Maybe I
will try to do some instrumentation with systemtap to show this (it's
always nice to find some uses for systemtap). Anyway I'm sure that
it's always better to not loose chars being received than delaying
transmitted one for a bit.

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

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

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[Alan Cox]

> 1) latency on rx chars becomes very high because we can process
> incoming transfers only after a full 8 byte (or whatever the spi
> transfer dimension is). For a 9600 this is too much.

There I would partly disagree. Fixing the spi driver clearly makes sense
but the serial driver should be batching better. Is there a reason the
driver couldn't adjust the size based upon the tty speed when getting a
termios request ?

> 2) even worse is that we can do flow control decision only on such boundary.

For serial flow control it doesn't matter, its implicitly asynchronous
and if you only turn the fifo on at high speed you response time will be
reasonably bounded.

> 3) this is not reliable: think of what happens if the actual SPI
> transfer speed we get will be slower that the one we requested. We
> won't be emptying the RX buffer fastly enough even if could.

Consoles are not usually balanced for I/O. I grant you probably don't
want to be using full fifo sized blocks but I'm not sure I understand why
there is a problem below that ?

Alan

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[christian pellegrin]

On Tue, Mar 30, 2010 at 10:46 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> There I would partly disagree. Fixing the spi driver clearly makes sense
> but the serial driver should be batching better. Is there a reason the
> driver couldn't adjust the size based upon the tty speed when getting a
> termios request ?

Just a small note for those who don't know max31x0. There are just 2
instructions to tx/rx data: simple data rx (in which we read one data
from the fifo (the data is valid if R status bit is set), control
lines (CTS) and status bits) and a combined tx/rx (like before but we
also send one byte (if the T status bit the char will be accepted) and
set control lines (RTS)). Every of these instructions is 2 bytes long.
It's a full SPI message (so you have to assert CS before and de-assert
it afterwards to make it work). TX buffer is just 1 byte deep, RX one
is 8 bytes.

Unfortunately we cannot batch TX if SPI speed (in chars going in /
time unit) is higher than the baud rate of serial communication. We
need to check that T bis is asserted before sending a char, otherwise
it is lost. For low baud rates doing this means monopolizing the bus.
And there is a subtle problem if we reduce SPI speed to the UART baud
rate: we can ask only for a maximum speed on the SPI. In case this is
lower the one we asked (maybe because of SPI clock divisors) and we do
a TX batch big enough we could not be emptying the RX FIFO fast enough
and so we risk to loose chars in a full duplex operation (*).

We could do batch RX operation but perhaps we are just transferring
bytes for nothing because we don't know in advance if the R bit is
set. And we cannot interleave TX and RX like current drivers do
because we are forced to use the simple rx operation and not the
combined rx/tx.

There's another point about batching: single instructions are complete
SPI messages. Many SPI master drivers I worked with (S3C24xx, AT91
because of a bug in the hardware) manage CS as a normal gpio. Another
problem here is that many devices require a configurable delay on CS
change (see delay_usecs field of spi_transfer) which is not handle by
hardware and so the drivers must somehow break a SPI messages in
single transfers and do something at the end of every one of these. So
even we batch 10 tx/rx instructions I really don't think many SPI
master controllers will be able to do a DMA of 20 bytes. Anyway I
agree that if we find a way of doing batching it's a good thing
because better controllers can benefit.

>
>> 2) even worse is that we can do flow control decision only on such boundary.
>
> For serial flow control it doesn't matter, its implicitly asynchronous
> and if you only turn the fifo on at high speed you response time will be
> reasonably bounded.
>

if we somehow manage to batch a TX of n bytes and after the first one
CTS changes we are going to send n-1 chars anyway. OK, maybe our peer
de-asserted CTS when still having n-1 byes of buffer free and somehow
higher layer protocols will recover. So I agree this could be not so
worrisome. But we should keep n small enough.

>> 3) this is not reliable: think of what happens if the actual SPI
>> transfer speed we get will be slower that the one we requested. We
>> won't be emptying the RX buffer fastly enough even if could.
>
> Consoles are not usually balanced for I/O. I grant you probably don't
> want to be using full fifo sized blocks but I'm not sure I understand why
> there is a problem below that ?
>

My concern is expressed above (*). Perhaps it's me missing some point.
To make it clear: I'm more than happy to test a driver that takes
another approach and switch to it if it's better (but it has to have
support of multiple instances and a fair usage of SPI bandwidth at
least). But I really don't see a reliable way of adding batching of
tx/rx. And I think it will be better to provide a patch to the current
driver than to rewrite it completely.

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

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[Grant Likely]

On Tue, Mar 30, 2010 at 6:03 AM, christian pellegrin <chripell@fsfe.org> wrote:
> On Tue, Mar 30, 2010 at 10:46 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>>
>> There I would partly disagree. Fixing the spi driver clearly makes sense
>> but the serial driver should be batching better. Is there a reason the
>> driver couldn't adjust the size based upon the tty speed when getting a
>> termios request ?
>
> Just a small note for those who don't know max31x0. There are just 2
> instructions to tx/rx data: simple data rx (in which we read one data
> from the fifo (the data is valid if R status bit is set), control
> lines (CTS) and status bits) and a combined tx/rx (like before but we
> also send one byte (if the T status bit the char will be accepted) and
> set control lines (RTS)). Every of these instructions is 2 bytes long.
> It's a full SPI message (so you have to assert CS before and de-assert
> it afterwards to make it work). TX buffer is just 1 byte deep, RX one
> is 8 bytes.

Is checking the T bit really necessary if Linux instead tracks the
timing between bytes internally?  ie.  If the driver uses an hrtimer
to schedule the submission of SPI transfers such that the time between
SPI submissions is always greater than the time required for a serial
character to be transmitted?

You may be able to set this up into a free-running state machine.
Submit the SPI transfers asynchronously, and use a callback to be
notified when the transfer is completed.  In most cases, the transfer
will be completed every time, but if it is not, then the state machine
can just wait another time period before submitting.  By doing it this
way, all the work can be handled at the atomic context without any
workqueues or threaded IRQ handlers.

Here's some draft )completely untested and uncompiled) code for the
bottom half:  The time rate would be calculated when the baud rate is
set.  I've glossed over a bunch of details and corner cases, but you
get the idea...

void max3100_spi_complete(void *__max3100)
{
  struct max3100_port *max3100 = __max3100;
  spin_lock(&max3100->lock);
  max3100->busy = 0;
  /* Process received characters here.  Schedule a workqueue if
needed.  A ring buffer would probably work well. */
  spin_unlock(&max3100->lock);
}

enum hrtimer_restart max3100_hrtimer_callback( struct hrtimer *timer )
{
  struct max3100_port *max3100 = container_of(timer, struct
max3100_port, hrtimer);

  if (max3100->busy)
    return HRTIMER_RESTART; /* still busy, try again later */

  spin_lock(&max3100->lock);
  /* The following 7 lines can probably actually be done once at
driver probe time */
  spi_message_init(&max3100->spi_msg);
  spi_message_add_tail(&max3100->spi_tran);
  max3100->spi_tran.tx_buf = mac3100->tx_buf;
  max3100->spi_tran.rx_buf = mac3100->rx_buf;
  max3100->spi_tran.len = 2;  /* can this be larger to receive
multiple bytes in 1 transfer? */
  max3100->spi_msg.complete = max3100_spi_complete;
  max3100->spi_msg.context = max3100;
  /* Alternately, a bunch of transfers could be queued up with only
the first actual
   * containing tx data.  The rest would be for RX */

  if (&max3100->tx_pending) {
    /* a ring buffer may be a better idea here */
    max3100->tx_buf[0] = max3100->tx_pending_data;
    max3100->tx_pending = 0;
  }
  spi_async(&max3100->spi_msg);
  max3100->busy = 1;  /* cleared by the completion callback */

  spin_unlock(&max3100->lock);

  return HRTIMER_RESTART;
}

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[christian pellegrin]

Hi,

first of all I'm sorry for the late response, just now I found time to
work on this.

On Wed, Mar 31, 2010 at 8:04 AM, Grant Likely <grant.likely@secretlab.ca> wrote:

> Is checking the T bit really necessary if Linux instead tracks the
> timing between bytes internally?  ie.  If the driver uses an hrtimer
> to schedule the submission of SPI transfers such that the time between
> SPI submissions is always greater than the time required for a serial
> character to be transmitted?

I think you have to check it anyway. For example the SPI bus may be
shared with another device so we don't know when our char will be sent
(it might be delayed for more than the duration of a char being sent
on the serial line if the initial execution of the SPI command is
delayed). But using a hrtimer will be for sure more fair than polling
T bit as far as resource usage is concerned. I was always hesitant
about using hrtimers: I really don't know if all platforms support
them with the needed granularity (at 115200 a char takes around 100us)
and the aren't many users of them in the drivers directory (quite all
of them are in the staging one). But it's definitely a good idea if
hrtimers do work. I'll make some tests.

>
> You may be able to set this up into a free-running state machine.
> Submit the SPI transfers asynchronously, and use a callback to be
> notified when the transfer is completed.  In most cases, the transfer
> will be completed every time, but if it is not, then the state machine
> can just wait another time period before submitting.  By doing it this
> way, all the work can be handled at the atomic context without any
> workqueues or threaded IRQ handlers.
>

yes a completely async design could improve performance (the greatest
culprit for low performance (not mentioning slow SPI master drivers)
is the latency in the delayed work being started). When I first wrote
this driver I wanted to keep it simple so I was a bit frightened by a
state-machine like design, but it can be done for sure. My concern
here is: everything is the kernel is moving to doing as much as
possible in a delayed work mechanics (see the introduction of threaded
interrupts (which could became the default) or the "coming soon" death
of IRQF_DISABLED). Is doing a big part of the work (of course I would
use spi_async directly in the interrupt handler and handle the
incoming/outgoing chars in spi_async callback which is usually called
in an interrupt context) in an interrupt context "antihistorical",
isn't it?

BTW: both of the design changes you mentioned seem sensible to me for
better performance of the driver. But they don't do any form of
batching and won't help if the underlying SPI master driver uses some
form of delayed work itself.

-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[Grant Likely]

On Mon, Apr 5, 2010 at 12:19 PM, christian pellegrin <chripell@fsfe.org> wrote:
> Hi,
>
> first of all I'm sorry for the late response, just now I found time to
> work on this.
>
> On Wed, Mar 31, 2010 at 8:04 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>
>> Is checking the T bit really necessary if Linux instead tracks the
>> timing between bytes internally?  ie.  If the driver uses an hrtimer
>> to schedule the submission of SPI transfers such that the time between
>> SPI submissions is always greater than the time required for a serial
>> character to be transmitted?
>
> I think you have to check it anyway. For example the SPI bus may be
> shared with another device so we don't know when our char will be sent
> (it might be delayed for more than the duration of a char being sent
> on the serial line if the initial execution of the SPI command is
> delayed). But using a hrtimer will be for sure more fair than polling
> T bit as far as resource usage is concerned. I was always hesitant
> about using hrtimers: I really don't know if all platforms support
> them with the needed granularity (at 115200 a char takes around 100us)
> and the aren't many users of them in the drivers directory (quite all
> of them are in the staging one). But it's definitely a good idea if
> hrtimers do work. I'll make some tests.

Another thing to think about: this device is never going to be high
performance.  If tx gets delayed, then it isn't a big deal because
there won't be any data loss.  The real concern is RX where if you
don't process fast enough then characters get dropped.  Fortunately
you have an 8 byte deep buffer.  So if each timer fire schedules more
than one transfer (even if only one char is actually transmitted),
then it is okay if the timer granularity is 300-400us at 115200.

>
>>
>> You may be able to set this up into a free-running state machine.
>> Submit the SPI transfers asynchronously, and use a callback to be
>> notified when the transfer is completed.  In most cases, the transfer
>> will be completed every time, but if it is not, then the state machine
>> can just wait another time period before submitting.  By doing it this
>> way, all the work can be handled at the atomic context without any
>> workqueues or threaded IRQ handlers.
>>
>
> yes a completely async design could improve performance (the greatest
> culprit for low performance (not mentioning slow SPI master drivers)
> is the latency in the delayed work being started). When I first wrote
> this driver I wanted to keep it simple so I was a bit frightened by a
> state-machine like design, but it can be done for sure. My concern
> here is: everything is the kernel is moving to doing as much as
> possible in a delayed work mechanics (see the introduction of threaded
> interrupts (which could became the default) or the "coming soon" death
> of IRQF_DISABLED). Is doing a big part of the work (of course I would
> use spi_async directly in the interrupt handler and handle the
> incoming/outgoing chars in spi_async callback which is usually called
> in an interrupt context) in an interrupt context "antihistorical",
> isn't it?

If you only use spi_async (which must be atomic), and you don't do any
heavy data processing or udelay()s in the irq path, then your driver
will not be adding major latency.  Use a ring buffer for both the tx
and rx paths so that the console processing can be done in a workqueue
or something and you should be just fine.

> BTW: both of the design changes you mentioned seem sensible to me for
> better performance of the driver. But they don't do any form of
> batching and won't help if the underlying SPI master driver uses some
> form of delayed work itself.

Then the SPI master driver is broken.  It must be fixed.  :-)
Submission of SPI transfers via spi_async() is supposed to always be
atomic.

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

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[christian pellegrin]

On Mon, Mar 29, 2010 at 4:48 AM, Feng Tang <feng.tang@intel.com> wrote:

>> +             tx |= MAX3100_WD | MAX3100_RTS;
>
> Does this imply to have to work with HW flow control? on my platform
> I have to remove the RTS bit to make it work.
>

Finally I had time to check this. If you compare the 8250 (or similar)
data-sheet with the MAX31x0 one you see that the handling of the
RTX/CTS bits is exactly the same (8250 about RTS bit for example:
"When any of these bits are cleared, the associated output is forced
high." and MAX3110: "Request-to-Send Bit. Controls the state of the
RTS output. This bit is reset on power-up (RTS
bit = 0 sets the RTS pin = logic high)."). If you look at the 8250.c
driver (grep for UART_MCR_RTS) you notice that the bit is set on
device open (together with DTR of course). So I think the driver is
doing the right thing here.

-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/4] max3100: adds console support for MAX3100

[christian pellegrin]

On Thu, Apr 8, 2010 at 11:31 AM, christian pellegrin <chripell@fsfe.org> wrote:
> On Mon, Mar 29, 2010 at 4:48 AM, Feng Tang <feng.tang@intel.com> wrote:
>
>>> +             tx |= MAX3100_WD | MAX3100_RTS;
>>
>> Does this imply to have to work with HW flow control? on my platform
>> I have to remove the RTS bit to make it work.
>>
>
> Finally I had time to check this. If you compare the 8250 (or similar)
> data-sheet with the MAX31x0 one you see that the handling of the
> RTX/CTS bits is exactly the same (8250 about RTS bit for example:
> "When any of these bits are cleared, the associated output is forced
> high." and MAX3110: "Request-to-Send Bit. Controls the state of the
> RTS output. This bit is reset on power-up (RTS
> bit = 0 sets the RTS pin = logic high)."). If you look at the 8250.c
> driver (grep for UART_MCR_RTS) you notice that the bit is set on
> device open (together with DTR of course). So I think the driver is
> doing the right thing here.
>

anyway I'll set it to on only in case the flow control is enabled. So
even if, for some reason, you have to keep it to off on your platform,
it won't make troubles.


-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 4/4] max3100: introduced to_max3100_port, small style fixes

[Christian Pellegrin]

static inline to_max3100_port was introduced for clarity. Whitespace
fixes.

Signed-off-by: Christian Pellegrin <chripell@fsfe.org>
---
 drivers/serial/max3100.c |   77 +++++++++++++++-------------------------------
 1 files changed, 25 insertions(+), 52 deletions(-)

diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
index ec3e13a..6644222 100644
--- a/drivers/serial/max3100.c
+++ b/drivers/serial/max3100.c
@@ -131,7 +131,7 @@ struct max3100_port {
 	/* poll time (in ms) for ctrl lines */
 	int poll_time;
 	/* and its timer */
-	struct timer_list	timer;
+	struct timer_list timer;
 
 	int console_flags;
 	/* is this port a console? */
@@ -153,6 +153,11 @@ struct max3100_port {
 static struct max3100_port *max3100s[MAX_MAX3100]; /* the chips */
 static DEFINE_MUTEX(max3100s_lock);		   /* race on probe */
 
+static inline struct max3100_port *to_max3100_port(struct uart_port *port)
+{
+  return container_of(port, struct max3100_port, port);
+}
+
 static int max3100_do_parity(struct max3100_port *s, u16 c)
 {
 	int parity;
@@ -344,9 +349,7 @@ static irqreturn_t max3100_ist(int irq, void *dev_id)
 
 static void max3100_enable_ms(struct uart_port *port)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	if (s->poll_time > 0)
 		mod_timer(&s->timer, jiffies);
@@ -355,9 +358,7 @@ static void max3100_enable_ms(struct uart_port *port)
 
 static void max3100_start_tx(struct uart_port *port)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -366,9 +367,7 @@ static void max3100_start_tx(struct uart_port *port)
 
 static void max3100_stop_rx(struct uart_port *port)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -382,9 +381,7 @@ static void max3100_stop_rx(struct uart_port *port)
 
 static unsigned int max3100_tx_empty(struct uart_port *port)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -395,9 +392,7 @@ static unsigned int max3100_tx_empty(struct uart_port *port)
 
 static unsigned int max3100_get_mctrl(struct uart_port *port)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -409,9 +404,7 @@ static unsigned int max3100_get_mctrl(struct uart_port *port)
 
 static void max3100_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 	int rts;
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -431,9 +424,7 @@ static void
 max3100_set_termios(struct uart_port *port, struct ktermios *termios,
 		    struct ktermios *old)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 	int baud = 0;
 	unsigned cflag;
 	u32 param_new, param_mask, parity = 0;
@@ -557,9 +548,7 @@ max3100_set_termios(struct uart_port *port, struct ktermios *termios,
 
 static void max3100_shutdown(struct uart_port *port)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -586,9 +575,7 @@ static void max3100_shutdown(struct uart_port *port)
 
 static int max3100_startup(struct uart_port *port)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -600,7 +587,7 @@ static int max3100_startup(struct uart_port *port)
 	s->rts = 0;
 
 	if (request_threaded_irq(s->irq, NULL, max3100_ist,
-			IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
+				 IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
 		dev_err(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
 		s->irq = 0;
 		return -EBUSY;
@@ -626,9 +613,7 @@ static int max3100_startup(struct uart_port *port)
 
 static const char *max3100_type(struct uart_port *port)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -637,18 +622,14 @@ static const char *max3100_type(struct uart_port *port)
 
 static void max3100_release_port(struct uart_port *port)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 }
 
 static void max3100_config_port(struct uart_port *port, int flags)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -659,9 +640,7 @@ static void max3100_config_port(struct uart_port *port, int flags)
 static int max3100_verify_port(struct uart_port *port,
 			       struct serial_struct *ser)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 	int ret = -EINVAL;
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -673,18 +652,14 @@ static int max3100_verify_port(struct uart_port *port,
 
 static void max3100_stop_tx(struct uart_port *port)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 }
 
 static int max3100_request_port(struct uart_port *port)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 	return 0;
@@ -692,9 +667,7 @@ static int max3100_request_port(struct uart_port *port)
 
 static void max3100_break_ctl(struct uart_port *port, int break_state)
 {
-	struct max3100_port *s = container_of(port,
-					      struct max3100_port,
-					      port);
+	struct max3100_port *s = to_max3100_port(port);
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 }
@@ -1010,9 +983,9 @@ static int max3100_suspend(struct spi_device *spi, pm_message_t state)
 	s->console_flags |= MAX3100_SUSPENDING;
 	disable_irq(s->irq);
 
-	if (s->max3100_hw_suspend)
+	if (s->max3100_hw_suspend) {
 		s->max3100_hw_suspend(1);
-	else {
+	} else {
 		/* no HW suspend, so do SW one */
 		u16 tx, rx;
 
-- 
1.5.6.5

Re: [PATCH v1 1/4] max3100: added raise_threaded_irq

[Thomas Gleixner]

On Tue, 23 Mar 2010, Christian Pellegrin wrote:

> raise_threaded_irq schedules the execution of an interrupt thread

I really have a hard time to understand _WHY_ we want to have that
function.

Interrupt threads are woken up either by the primary handler or by a
interrupt demultiplexer and the code has all interfaces for that
already.

Can you please explain, what you are trying to achieve and why it
can't be done with the existing interfaces ?

> +
> +/**
> + *	raise_threaded_irq - triggers a threded interrupt
> + *	@irq: Interrupt line to trigger
> + */
> +int raise_threaded_irq(unsigned int irq)
> +{
> +	struct irq_desc *desc = irq_to_desc(irq);
> +	struct irqaction *action;
> +
> +	if (!desc)
> +		return -ENOENT;
> +	action = desc->action;

That's racy. You cannot access desc->action w/o holding desc->lock or
having set the IRQ_INPROGRESS flag in desc->status under desc->lock.

> +	if (!action)
> +		return -ENOENT;
> +	if (unlikely(!action->thread_fn))
> +		return -EINVAL;
> +	if (likely(!test_bit(IRQTF_DIED,
> +			     &action->thread_flags))) {
> +		set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
> +		wake_up_process(action->thread);
> +	} else {
> +		return -ECHILD;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(raise_threaded_irq);

EXPORT_SYMBOL_GPL if at all.

Aside of that the name of of the function sucks: irq_wake_thread()
perhaps ?

But I still have no idea why we would need it at all.

Thanks,

	tglx

Re: [PATCH v1 1/4] max3100: added raise_threaded_irq

[christian pellegrin]

Hi,

On Fri, Apr 16, 2010 at 1:22 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> raise_threaded_irq schedules the execution of an interrupt thread
>
> I really have a hard time to understand _WHY_ we want to have that
> function.
>
.....
>
> Can you please explain, what you are trying to achieve and why it
> can't be done with the existing interfaces ?
>

The idea was that by using this function we just need one kind of
deferred work (interrupt threads) instead of two (for example
interrupt threads and workqueues) in some situations. This is very
handy with devices that do transmission and reception at the same
time, for example many SPI devices. The user case is the max3100 UART
on SPI driver. The same SPI instruction both receives and sends chars.
So when we need to send a char we just start the interrupt thread
instead of having another kind of deferred work doing the job. This
greatly simplifies locking and avoids duplication of functionality
(otherwise we must have an interrupt thread that does reception and a
workqueue that does sending and receiving for example) because
everything is done in just one point. The move from worqueues to
interrupt threads was motivated by the much smaller latency under load
of the latter because they are scheduled as RT processes. I hope this
doesn't sound like a terrible abuse of threaded interrupts. Let me
know before I try to fix other problems you mentioned.

Thanks

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

Re: [PATCH v1 1/4] max3100: added raise_threaded_irq

[Thomas Gleixner]

Christian,

On Fri, 16 Apr 2010, christian pellegrin wrote:

> Hi,
> 
> On Fri, Apr 16, 2010 at 1:22 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> >> raise_threaded_irq schedules the execution of an interrupt thread
> >
> > I really have a hard time to understand _WHY_ we want to have that
> > function.
> >
> .....
> >
> > Can you please explain, what you are trying to achieve and why it
> > can't be done with the existing interfaces ?
> >
> 
> The idea was that by using this function we just need one kind of
> deferred work (interrupt threads) instead of two (for example
> interrupt threads and workqueues) in some situations. This is very
> handy with devices that do transmission and reception at the same
> time, for example many SPI devices. The user case is the max3100 UART
> on SPI driver. The same SPI instruction both receives and sends chars.
> So when we need to send a char we just start the interrupt thread
> instead of having another kind of deferred work doing the job. This
> greatly simplifies locking and avoids duplication of functionality
> (otherwise we must have an interrupt thread that does reception and a
> workqueue that does sending and receiving for example) because
> everything is done in just one point. The move from worqueues to
> interrupt threads was motivated by the much smaller latency under load
> of the latter because they are scheduled as RT processes. I hope this
> doesn't sound like a terrible abuse of threaded interrupts. Let me
> know before I try to fix other problems you mentioned.

Thanks for the explanation. Now, that makes a lot of sense and I can
see that it removes a lot of serialization issues and duplicated code
pathes.

So what you want is a mechanism to "inject" interrupts by
software. 

I wonder whether we should restrict this mechanism to threaded
handlers or just implement it in the following way:

int irq_inject(unsigned int irq)
{
        struct irq_desc *desc = irq_to_desc(irq);

	if (!desc)
		return -EINVAL;
	
	local_irq_disable();
        desc->handle_irq(irq, desc);
        local_irq_enable();
	return 0;
}

That would call the real interupt code path as it is called from the
arch/*/kernel/irq.c:entry code and take care of all serialization
issues.

The drawback is that it will increase the irq statistics, but I think
that's really a pure cosmetic problem.

That requires that the primary interrupt handler code knows about the
software "interrupt" event, but that's easy to solve. So the primary
handler would just return IRQ_WAKE_THREAD as it would do for a real
hardware triggered interrupt. But as a goodie that would work for non
threaded interrupts as well.

There is another thing which needs some care: 

This will not work out of the box when the irq is nested into some
demultiplexing thread handler (IRQF_NESTED_THREAD).

I'm too tried to look at this now, but I don't see a real showstopper
there.

Thanks,

	tglx

Re: [PATCH v1 1/4] max3100: added raise_threaded_irq

[christian pellegrin]

Hi,


On Sat, Apr 17, 2010 at 12:06 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> I wonder whether we should restrict this mechanism to threaded
> handlers or just implement it in the following way:
>
.....
> This will not work out of the box when the irq is nested into some
> demultiplexing thread handler (IRQF_NESTED_THREAD).
>

OK, I see. The solution you proposed has many other uses such testing
of interrupt handlers in general. I will try to do some work along the
line you suggested next week.

Thank you!


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