login
Search
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." --
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
--
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) &&
- ...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 ...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 Does this imply to have to work with HW flow control? on my platform 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. 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? --
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 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 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 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." --
On Mon, 29 Mar 2010 14:11:15 +0800 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 It's difficult but not impossible, actually our driver checks every word 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 --
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 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. 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." --
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." --
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." --
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 = ...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 That's racy. You cannot access desc->action w/o holding desc->lock or 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 --
Hi, 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." --
Christian,
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
--
Hi, 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." --
