[PATCH v1 0/3] max3100: improvements

[PATCH v1 0/3] max3100: improvements

[christian pellegrin]

Hi all,

I think all the comments sent to me were dealt with so I'm resending
the patches. I haven't received any feedback about the addition to the
threaded interrupt code, but that part hasn't changed.

This bunch of patches should solve the problems noted by Feng to the
max3100 driver. It was tested on an S3C2440 system which has a rather
bad SPI master controller. The efficiency is very good on zmodem
receive (near 100% at 115200) and not so bad on zmodem send (around
95% at 115200). I guess the reason is the TX buffer of the MAX3100
being just one byte deep. All the tests were done with two MAX3100
running on the same SPI bus: the first one as a console and under
test, the second one happily receiving a 4800 bit/s NMEA stream from a
GPS (with a process checking we don't get wrong sentences). I also did
the simple "cut&paste a screenful of charters" test and it worked! The
console was stressed as much I could (things like putting the S3C to
suspend during a zmodem transfer and checking it restarts after
resume).


-- 
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."

[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."

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

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

[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."

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."

[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."