The following patchset cleans up the atmel_serial driver a bit,
moves a significant portion of the interrupt handler into a tasklet,
and adds DMA support. This is the result of a combined effort by Chip
Coldwell, Remy Bohmer and me. The patches should apply cleanly onto
Linus' latest git tree, and I've also tested it on -mm (with a couple
of avr32 fixes applied to make the rest of the tree compile.)
With DMA, I see transfer rates around 92 kbps when transferring a big
file using ZModem (both directions are roughly the same.) I've also
tested the same thing with a bunch of debug options enabled. The
transfer rate is slightly lower, but no errors are reported.
Note that break and error handling doesn't work too well with DMA
enabled. This is a common problem with all the efforts I've seen
adding DMA support to this driver (including my own). The PDC error
handling also accesses icount without locking. I'm tempted to just
ignore the problem for now and hopefully come up with a solution
later.
I think this stuff is ready for -mm now, but I'd like to see a couple
more ACKs from Andrew Victor and other people who may be familiar with
this driver and/or the serial/tty layer in general.
Changes since v3:
* Added myself to MAINTAINERS
* Additional cleanups suggested by Andrew Victor
* Made PDC support configurable through Kconfig
* Converted lots of potentially unsafe casts to use container_of()
* Show tty name (not just "atmel_serial") in /proc/interrupts
Chip Coldwell (1):
atmel_serial: Add DMA support
Haavard Skinnemoen (6):
MAINTAINERS: Add myself as maintainer of the atmel_serial driver
atmel_serial: Use cpu_relax() when busy-waiting
atmel_serial: Use existing console options only if BRG is running
atmel_serial: Fix bugs in probe() error path and remove()
atmel_serial: Use container_of instead of direct cast
atmel_serial: Show tty name in /proc/interrupts
Remy Bohmer (2):
atmel_serial: Clean up the code
atmel_serial: Split the interrupt handler
MAINTAINERS | 6 +
drivers/serial/Kconfig | 15 +
drivers/serial/atmel_serial.c | 888 +++++++++++++++++++++++++++++++++--------
3 files changed, 740 insertions(+), 169 deletions(-)
The atmel_serial driver never had a MAINTAINERS entry, although Andrew
Victor has effectively been acting as a maintainer since he got the
driver merged into mainline in the first place.
I'll keep Cc'ing Andrew on all patches, but I'm going to take the
main responsibility for getting things moving upstream from now on.
Signed-off-by: Haavard Skinnemoen <[email protected]>
Acked-by: Andrew Victor <[email protected]>
---
MAINTAINERS | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 2340cfb..e349a9e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -671,6 +671,12 @@ W: http://www.atmel.com/products/AT91/
W: http://www.at91.com/
S: Maintained
+ATMEL AT91 / AT32 SERIAL DRIVER
+P: Haavard Skinnemoen
+M: [email protected]
+L: [email protected]
+S: Supported
+
ATMEL LCDFB DRIVER
P: Nicolas Ferre
M: [email protected]
--
1.5.3.8
When an error happens in probe(), the clocks should be disabled, but
only if the port isn't already used as a console.
In remove(), the port struct shouldn't be freed because it's defined
statically.
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
drivers/serial/atmel_serial.c | 39 +++++++++++++++++++++++++++++----------
1 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 746aea0..0e715f4 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -935,8 +935,18 @@ static int __init atmel_late_console_init(void)
core_initcall(atmel_late_console_init);
+static inline bool atmel_is_console_port(struct uart_port *port)
+{
+ return port->cons && port->cons->index == port->line;
+}
+
#else
#define ATMEL_CONSOLE_DEVICE NULL
+
+static inline bool atmel_is_console_port(struct uart_port *port)
+{
+ return false;
+}
#endif
static struct uart_driver atmel_uart = {
@@ -994,9 +1004,19 @@ static int __devinit atmel_serial_probe(struct platform_device *pdev)
atmel_init_port(port, pdev);
ret = uart_add_one_port(&atmel_uart, &port->uart);
- if (!ret) {
- device_init_wakeup(&pdev->dev, 1);
- platform_set_drvdata(pdev, port);
+ if (ret)
+ goto err_add_port;
+
+ device_init_wakeup(&pdev->dev, 1);
+ platform_set_drvdata(pdev, port);
+
+ return 0;
+
+err_add_port:
+ if (!atmel_is_console_port(&port->uart)) {
+ clk_disable(port->clk);
+ clk_put(port->clk);
+ port->clk = NULL;
}
return ret;
@@ -1008,16 +1028,15 @@ static int __devexit atmel_serial_remove(struct platform_device *pdev)
struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
int ret = 0;
- clk_disable(atmel_port->clk);
- clk_put(atmel_port->clk);
-
device_init_wakeup(&pdev->dev, 0);
platform_set_drvdata(pdev, NULL);
- if (port) {
- ret = uart_remove_one_port(&atmel_uart, port);
- kfree(port);
- }
+ ret = uart_remove_one_port(&atmel_uart, port);
+
+ /* "port" is allocated statically, so we shouldn't free it */
+
+ clk_disable(atmel_port->clk);
+ clk_put(atmel_port->clk);
return ret;
}
--
1.5.3.8
As pointed out by David Brownell, we really ought to be using
container_of when converting from "struct uart_port *" to "struct
atmel_uart_port *".
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
drivers/serial/atmel_serial.c | 40 +++++++++++++++++++++++-----------------
1 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index e4f9449..2ff92b9 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -157,17 +157,23 @@ static struct atmel_uart_port atmel_ports[ATMEL_MAX_UART];
static struct console atmel_console;
#endif
+static inline struct atmel_uart_port *
+to_atmel_uart_port(struct uart_port *uart)
+{
+ return container_of(uart, struct atmel_uart_port, uart);
+}
+
#ifdef CONFIG_SERIAL_ATMEL_PDC
static bool atmel_use_dma_rx(struct uart_port *port)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
return atmel_port->use_dma_rx;
}
static bool atmel_use_dma_tx(struct uart_port *port)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
return atmel_port->use_dma_tx;
}
@@ -330,7 +336,7 @@ static void
atmel_buffer_rx_char(struct uart_port *port, unsigned int status,
unsigned int ch)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
struct circ_buf *ring = &atmel_port->rx_ring;
struct atmel_uart_char *c;
@@ -374,7 +380,7 @@ static void atmel_pdc_rxerr(struct uart_port *port, unsigned int status)
*/
static void atmel_rx_chars(struct uart_port *port)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
unsigned int status, ch;
status = UART_GET_CSR(port);
@@ -454,7 +460,7 @@ static void atmel_tx_chars(struct uart_port *port)
static void
atmel_handle_receive(struct uart_port *port, unsigned int pending)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
if (atmel_use_dma_rx(port)) {
/*
@@ -495,7 +501,7 @@ atmel_handle_receive(struct uart_port *port, unsigned int pending)
static void
atmel_handle_transmit(struct uart_port *port, unsigned int pending)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
if (atmel_use_dma_tx(port)) {
/* PDC transmit */
@@ -519,7 +525,7 @@ static void
atmel_handle_status(struct uart_port *port, unsigned int pending,
unsigned int status)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
if (pending & (ATMEL_US_RIIC | ATMEL_US_DSRIC | ATMEL_US_DCDIC
| ATMEL_US_CTSIC)) {
@@ -555,7 +561,7 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id)
*/
static void atmel_tx_dma(struct uart_port *port)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
struct circ_buf *xmit = &port->info->xmit;
struct atmel_dma_buffer *pdc = &atmel_port->pdc_tx;
int count;
@@ -601,7 +607,7 @@ static void atmel_tx_dma(struct uart_port *port)
static void atmel_rx_from_ring(struct uart_port *port)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
struct circ_buf *ring = &atmel_port->rx_ring;
unsigned int flg;
unsigned int status;
@@ -669,7 +675,7 @@ static void atmel_rx_from_ring(struct uart_port *port)
static void atmel_rx_from_dma(struct uart_port *port)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
struct tty_struct *tty = port->info->tty;
struct atmel_dma_buffer *pdc;
int rx_idx = atmel_port->pdc_rx_idx;
@@ -743,7 +749,7 @@ static void atmel_rx_from_dma(struct uart_port *port)
static void atmel_tasklet_func(unsigned long data)
{
struct uart_port *port = (struct uart_port *)data;
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
unsigned int status;
unsigned int status_change;
@@ -788,7 +794,7 @@ static void atmel_tasklet_func(unsigned long data)
*/
static int atmel_startup(struct uart_port *port)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
int retval;
/*
@@ -898,7 +904,7 @@ static int atmel_startup(struct uart_port *port)
*/
static void atmel_shutdown(struct uart_port *port)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
/*
* Ensure everything is stopped.
*/
@@ -955,7 +961,7 @@ static void atmel_shutdown(struct uart_port *port)
static void atmel_serial_pm(struct uart_port *port, unsigned int state,
unsigned int oldstate)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
switch (state) {
case 0:
@@ -1427,7 +1433,7 @@ static int atmel_serial_suspend(struct platform_device *pdev,
pm_message_t state)
{
struct uart_port *port = platform_get_drvdata(pdev);
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
if (device_may_wakeup(&pdev->dev)
&& !at91_suspend_entering_slow_clock())
@@ -1443,7 +1449,7 @@ static int atmel_serial_suspend(struct platform_device *pdev,
static int atmel_serial_resume(struct platform_device *pdev)
{
struct uart_port *port = platform_get_drvdata(pdev);
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
if (atmel_port->suspended) {
uart_resume_port(&atmel_uart, port);
@@ -1502,7 +1508,7 @@ err_alloc_ring:
static int __devexit atmel_serial_remove(struct platform_device *pdev)
{
struct uart_port *port = platform_get_drvdata(pdev);
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
int ret = 0;
device_init_wakeup(&pdev->dev, 0);
--
1.5.3.8
From: Remy Bohmer <[email protected]>
This patch cleans up the atmel_serial driver to conform the coding rules.
It contains no functional change.
Signed-off-by: Remy Bohmer <[email protected]>
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
drivers/serial/atmel_serial.c | 255 ++++++++++++++++++++++++-----------------
1 files changed, 150 insertions(+), 105 deletions(-)
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 111da57..ee5d844 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -74,6 +74,7 @@
#define ATMEL_ISR_PASS_LIMIT 256
+/* UART registers. CR is write-only, hence no GET macro */
#define UART_PUT_CR(port,v) __raw_writel(v, (port)->membase + ATMEL_US_CR)
#define UART_GET_MR(port) __raw_readl((port)->membase + ATMEL_US_MR)
#define UART_PUT_MR(port,v) __raw_writel(v, (port)->membase + ATMEL_US_MR)
@@ -87,8 +88,6 @@
#define UART_PUT_BRGR(port,v) __raw_writel(v, (port)->membase + ATMEL_US_BRGR)
#define UART_PUT_RTOR(port,v) __raw_writel(v, (port)->membase + ATMEL_US_RTOR)
-// #define UART_GET_CR(port) __raw_readl((port)->membase + ATMEL_US_CR) // is write-only
-
/* PDC registers */
#define UART_PUT_PTCR(port,v) __raw_writel(v, (port)->membase + ATMEL_PDC_PTCR)
#define UART_GET_PTSR(port) __raw_readl((port)->membase + ATMEL_PDC_PTSR)
@@ -101,8 +100,6 @@
#define UART_PUT_TPR(port,v) __raw_writel(v, (port)->membase + ATMEL_PDC_TPR)
#define UART_PUT_TCR(port,v) __raw_writel(v, (port)->membase + ATMEL_PDC_TCR)
-//#define UART_PUT_TNPR(port,v) __raw_writel(v, (port)->membase + ATMEL_PDC_TNPR)
-//#define UART_PUT_TNCR(port,v) __raw_writel(v, (port)->membase + ATMEL_PDC_TNCR)
static int (*atmel_open_hook)(struct uart_port *);
static void (*atmel_close_hook)(struct uart_port *);
@@ -142,8 +139,8 @@ static void atmel_set_mctrl(struct uart_port *port, u_int mctrl)
#ifdef CONFIG_ARCH_AT91RM9200
if (cpu_is_at91rm9200()) {
/*
- * AT91RM9200 Errata #39: RTS0 is not internally connected to PA21.
- * We need to drive the pin manually.
+ * AT91RM9200 Errata #39: RTS0 is not internally connected
+ * to PA21. We need to drive the pin manually.
*/
if (port->mapbase == AT91RM9200_BASE_US0) {
if (mctrl & TIOCM_RTS)
@@ -228,7 +225,8 @@ static void atmel_stop_rx(struct uart_port *port)
*/
static void atmel_enable_ms(struct uart_port *port)
{
- UART_PUT_IER(port, ATMEL_US_RIIC | ATMEL_US_DSRIC | ATMEL_US_DCDIC | ATMEL_US_CTSIC);
+ UART_PUT_IER(port, ATMEL_US_RIIC | ATMEL_US_DSRIC
+ | ATMEL_US_DCDIC | ATMEL_US_CTSIC);
}
/*
@@ -247,7 +245,7 @@ static void atmel_break_ctl(struct uart_port *port, int break_state)
*/
static void atmel_rx_chars(struct uart_port *port)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
struct tty_struct *tty = port->info->tty;
unsigned int status, ch, flg;
@@ -266,10 +264,12 @@ static void atmel_rx_chars(struct uart_port *port)
if (unlikely(status & (ATMEL_US_PARE | ATMEL_US_FRAME
| ATMEL_US_OVRE | ATMEL_US_RXBRK)
|| atmel_port->break_active)) {
- UART_PUT_CR(port, ATMEL_US_RSTSTA); /* clear error */
+ /* clear error */
+ UART_PUT_CR(port, ATMEL_US_RSTSTA);
if (status & ATMEL_US_RXBRK
&& !atmel_port->break_active) {
- status &= ~(ATMEL_US_PARE | ATMEL_US_FRAME); /* ignore side-effect */
+ /* ignore side-effect */
+ status &= ~(ATMEL_US_PARE | ATMEL_US_FRAME);
port->icount.brk++;
atmel_port->break_active = 1;
UART_PUT_IER(port, ATMEL_US_RXBRK);
@@ -309,7 +309,7 @@ static void atmel_rx_chars(struct uart_port *port)
uart_insert_char(port, status, ATMEL_US_OVRE, ch, flg);
- ignore_char:
+ignore_char:
status = UART_GET_CSR(port);
}
@@ -350,44 +350,73 @@ static void atmel_tx_chars(struct uart_port *port)
}
/*
+ * receive interrupt handler.
+ */
+static void
+atmel_handle_receive(struct uart_port *port, unsigned int pending)
+{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+
+ /* Interrupt receive */
+ if (pending & ATMEL_US_RXRDY)
+ atmel_rx_chars(port);
+ else if (pending & ATMEL_US_RXBRK) {
+ /*
+ * End of break detected. If it came along with a
+ * character, atmel_rx_chars will handle it.
+ */
+ UART_PUT_CR(port, ATMEL_US_RSTSTA);
+ UART_PUT_IDR(port, ATMEL_US_RXBRK);
+ atmel_port->break_active = 0;
+ }
+}
+
+/*
+ * transmit interrupt handler.
+ */
+static void
+atmel_handle_transmit(struct uart_port *port, unsigned int pending)
+{
+ /* Interrupt transmit */
+ if (pending & ATMEL_US_TXRDY)
+ atmel_tx_chars(port);
+}
+
+/*
+ * status flags interrupt handler.
+ */
+static void
+atmel_handle_status(struct uart_port *port, unsigned int pending,
+ unsigned int status)
+{
+ /* TODO: All reads to CSR will clear these interrupts! */
+ if (pending & ATMEL_US_RIIC)
+ port->icount.rng++;
+ if (pending & ATMEL_US_DSRIC)
+ port->icount.dsr++;
+ if (pending & ATMEL_US_DCDIC)
+ uart_handle_dcd_change(port, !(status & ATMEL_US_DCD));
+ if (pending & ATMEL_US_CTSIC)
+ uart_handle_cts_change(port, !(status & ATMEL_US_CTS));
+ if (pending & (ATMEL_US_RIIC | ATMEL_US_DSRIC | ATMEL_US_DCDIC
+ | ATMEL_US_CTSIC))
+ wake_up_interruptible(&port->info->delta_msr_wait);
+}
+
+/*
* Interrupt handler
*/
static irqreturn_t atmel_interrupt(int irq, void *dev_id)
{
struct uart_port *port = dev_id;
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
unsigned int status, pending, pass_counter = 0;
status = UART_GET_CSR(port);
pending = status & UART_GET_IMR(port);
while (pending) {
- /* Interrupt receive */
- if (pending & ATMEL_US_RXRDY)
- atmel_rx_chars(port);
- else if (pending & ATMEL_US_RXBRK) {
- /*
- * End of break detected. If it came along
- * with a character, atmel_rx_chars will
- * handle it.
- */
- UART_PUT_CR(port, ATMEL_US_RSTSTA);
- UART_PUT_IDR(port, ATMEL_US_RXBRK);
- atmel_port->break_active = 0;
- }
-
- // TODO: All reads to CSR will clear these interrupts!
- if (pending & ATMEL_US_RIIC) port->icount.rng++;
- if (pending & ATMEL_US_DSRIC) port->icount.dsr++;
- if (pending & ATMEL_US_DCDIC)
- uart_handle_dcd_change(port, !(status & ATMEL_US_DCD));
- if (pending & ATMEL_US_CTSIC)
- uart_handle_cts_change(port, !(status & ATMEL_US_CTS));
- if (pending & (ATMEL_US_RIIC | ATMEL_US_DSRIC | ATMEL_US_DCDIC | ATMEL_US_CTSIC))
- wake_up_interruptible(&port->info->delta_msr_wait);
-
- /* Interrupt transmit */
- if (pending & ATMEL_US_TXRDY)
- atmel_tx_chars(port);
+ atmel_handle_receive(port, pending);
+ atmel_handle_status(port, pending, status);
+ atmel_handle_transmit(port, pending);
if (pass_counter++ > ATMEL_ISR_PASS_LIMIT)
break;
@@ -415,7 +444,8 @@ static int atmel_startup(struct uart_port *port)
/*
* Allocate the IRQ
*/
- retval = request_irq(port->irq, atmel_interrupt, IRQF_SHARED, "atmel_serial", port);
+ retval = request_irq(port->irq, atmel_interrupt, IRQF_SHARED,
+ "atmel_serial", port);
if (retval) {
printk("atmel_serial: atmel_startup - Can't get irq\n");
return retval;
@@ -437,9 +467,11 @@ static int atmel_startup(struct uart_port *port)
* Finally, enable the serial port
*/
UART_PUT_CR(port, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
- UART_PUT_CR(port, ATMEL_US_TXEN | ATMEL_US_RXEN); /* enable xmit & rcvr */
+ /* enable xmit & rcvr */
+ UART_PUT_CR(port, ATMEL_US_TXEN | ATMEL_US_RXEN);
- UART_PUT_IER(port, ATMEL_US_RXRDY); /* enable receive only */
+ /* enable receive only */
+ UART_PUT_IER(port, ATMEL_US_RXRDY);
return 0;
}
@@ -471,45 +503,48 @@ static void atmel_shutdown(struct uart_port *port)
/*
* Power / Clock management.
*/
-static void atmel_serial_pm(struct uart_port *port, unsigned int state, unsigned int oldstate)
+static void atmel_serial_pm(struct uart_port *port, unsigned int state,
+ unsigned int oldstate)
{
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
switch (state) {
- case 0:
- /*
- * Enable the peripheral clock for this serial port.
- * This is called on uart_open() or a resume event.
- */
- clk_enable(atmel_port->clk);
- break;
- case 3:
- /*
- * Disable the peripheral clock for this serial port.
- * This is called on uart_close() or a suspend event.
- */
- clk_disable(atmel_port->clk);
- break;
- default:
- printk(KERN_ERR "atmel_serial: unknown pm %d\n", state);
+ case 0:
+ /*
+ * Enable the peripheral clock for this serial port.
+ * This is called on uart_open() or a resume event.
+ */
+ clk_enable(atmel_port->clk);
+ break;
+ case 3:
+ /*
+ * Disable the peripheral clock for this serial port.
+ * This is called on uart_close() or a suspend event.
+ */
+ clk_disable(atmel_port->clk);
+ break;
+ default:
+ printk(KERN_ERR "atmel_serial: unknown pm %d\n", state);
}
}
/*
* Change the port parameters
*/
-static void atmel_set_termios(struct uart_port *port, struct ktermios * termios, struct ktermios * old)
+static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
+ struct ktermios *old)
{
unsigned long flags;
unsigned int mode, imr, quot, baud;
/* Get current mode register */
- mode = UART_GET_MR(port) & ~(ATMEL_US_USCLKS | ATMEL_US_CHRL | ATMEL_US_NBSTOP | ATMEL_US_PAR);
+ mode = UART_GET_MR(port) & ~(ATMEL_US_USCLKS | ATMEL_US_CHRL
+ | ATMEL_US_NBSTOP | ATMEL_US_PAR);
- baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/16);
+ baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk / 16);
quot = uart_get_divisor(port, baud);
- if (quot > 65535) { /* BRGR is 16-bit, so switch to slower clock */
+ if (quot > 65535) { /* BRGR is 16-bit, so switch to slower clock */
quot /= 8;
mode |= ATMEL_US_USCLKS_MCK_DIV8;
}
@@ -536,18 +571,17 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios * termios,
/* parity */
if (termios->c_cflag & PARENB) {
- if (termios->c_cflag & CMSPAR) { /* Mark or Space parity */
+ /* Mark or Space parity */
+ if (termios->c_cflag & CMSPAR) {
if (termios->c_cflag & PARODD)
mode |= ATMEL_US_PAR_MARK;
else
mode |= ATMEL_US_PAR_SPACE;
- }
- else if (termios->c_cflag & PARODD)
+ } else if (termios->c_cflag & PARODD)
mode |= ATMEL_US_PAR_ODD;
else
mode |= ATMEL_US_PAR_EVEN;
- }
- else
+ } else
mode |= ATMEL_US_PAR_NONE;
spin_lock_irqsave(&port->lock, flags);
@@ -573,16 +607,16 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios * termios,
if (termios->c_iflag & IGNPAR)
port->ignore_status_mask |= ATMEL_US_OVRE;
}
-
- // TODO: Ignore all characters if CREAD is set.
+ /* TODO: Ignore all characters if CREAD is set.*/
/* update the per-port timeout */
uart_update_timeout(port, termios->c_cflag, baud);
- /* disable interrupts and drain transmitter */
- imr = UART_GET_IMR(port); /* get interrupt mask */
- UART_PUT_IDR(port, -1); /* disable all interrupts */
- while (!(UART_GET_CSR(port) & ATMEL_US_TXEMPTY)) { barrier(); }
+ /* save/disable interrupts and drain transmitter */
+ imr = UART_GET_IMR(port);
+ UART_PUT_IDR(port, -1);
+ while (!(UART_GET_CSR(port) & ATMEL_US_TXEMPTY))
+ barrier();
/* disable receiver and transmitter */
UART_PUT_CR(port, ATMEL_US_TXDIS | ATMEL_US_RXDIS);
@@ -708,7 +742,8 @@ static struct uart_ops atmel_pops = {
/*
* Configure the port from the platform device resource info.
*/
-static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port, struct platform_device *pdev)
+static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
+ struct platform_device *pdev)
{
struct uart_port *port = &atmel_port->uart;
struct atmel_uart_data *data = pdev->dev.platform_data;
@@ -731,7 +766,8 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port, struct
port->membase = NULL;
}
- if (!atmel_port->clk) { /* for console, the clock could already be configured */
+ /* for console, the clock could already be configured */
+ if (!atmel_port->clk) {
atmel_port->clk = clk_get(&pdev->dev, "usart");
clk_enable(atmel_port->clk);
port->uartclk = clk_get_rate(atmel_port->clk);
@@ -755,7 +791,6 @@ void __init atmel_register_uart_fns(struct atmel_port_fns *fns)
atmel_pops.set_wake = fns->set_wake;
}
-
#ifdef CONFIG_SERIAL_ATMEL_CONSOLE
static void atmel_console_putchar(struct uart_port *port, int ch)
{
@@ -773,28 +808,30 @@ static void atmel_console_write(struct console *co, const char *s, u_int count)
unsigned int status, imr;
/*
- * First, save IMR and then disable interrupts
+ * First, save IMR and then disable interrupts
*/
- imr = UART_GET_IMR(port); /* get interrupt mask */
+ imr = UART_GET_IMR(port);
UART_PUT_IDR(port, ATMEL_US_RXRDY | ATMEL_US_TXRDY);
uart_console_write(port, s, count, atmel_console_putchar);
/*
- * Finally, wait for transmitter to become empty
- * and restore IMR
+ * Finally, wait for transmitter to become empty
+ * and restore IMR
*/
do {
status = UART_GET_CSR(port);
} while (!(status & ATMEL_US_TXRDY));
- UART_PUT_IER(port, imr); /* set interrupts back the way they were */
+ /* set interrupts back the way they were */
+ UART_PUT_IER(port, imr);
}
/*
- * If the port was already initialised (eg, by a boot loader), try to determine
- * the current setup.
+ * If the port was already initialised (eg, by a boot loader),
+ * try to determine the current setup.
*/
-static void __init atmel_console_get_options(struct uart_port *port, int *baud, int *parity, int *bits)
+static void __init atmel_console_get_options(struct uart_port *port, int *baud,
+ int *parity, int *bits)
{
unsigned int mr, quot;
@@ -836,10 +873,12 @@ static int __init atmel_console_setup(struct console *co, char *options)
int parity = 'n';
int flow = 'n';
- if (port->membase == 0) /* Port not initialized yet - delay setup */
+ if (port->membase == NULL) {
+ /* Port not initialized yet - delay setup */
return -ENODEV;
+ }
- UART_PUT_IDR(port, -1); /* disable interrupts */
+ UART_PUT_IDR(port, -1);
UART_PUT_CR(port, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
UART_PUT_CR(port, ATMEL_US_TXEN | ATMEL_US_RXEN);
@@ -871,13 +910,16 @@ static struct console atmel_console = {
static int __init atmel_console_init(void)
{
if (atmel_default_console_device) {
- add_preferred_console(ATMEL_DEVICENAME, atmel_default_console_device->id, NULL);
- atmel_init_port(&(atmel_ports[atmel_default_console_device->id]), atmel_default_console_device);
+ add_preferred_console(ATMEL_DEVICENAME,
+ atmel_default_console_device->id, NULL);
+ atmel_init_port(&atmel_ports[atmel_default_console_device->id],
+ atmel_default_console_device);
register_console(&atmel_console);
}
return 0;
}
+
console_initcall(atmel_console_init);
/*
@@ -885,11 +927,13 @@ console_initcall(atmel_console_init);
*/
static int __init atmel_late_console_init(void)
{
- if (atmel_default_console_device && !(atmel_console.flags & CON_ENABLED))
+ if (atmel_default_console_device
+ && !(atmel_console.flags & CON_ENABLED))
register_console(&atmel_console);
return 0;
}
+
core_initcall(atmel_late_console_init);
#else
@@ -897,22 +941,24 @@ core_initcall(atmel_late_console_init);
#endif
static struct uart_driver atmel_uart = {
- .owner = THIS_MODULE,
- .driver_name = "atmel_serial",
- .dev_name = ATMEL_DEVICENAME,
- .major = SERIAL_ATMEL_MAJOR,
- .minor = MINOR_START,
- .nr = ATMEL_MAX_UART,
- .cons = ATMEL_CONSOLE_DEVICE,
+ .owner = THIS_MODULE,
+ .driver_name = "atmel_serial",
+ .dev_name = ATMEL_DEVICENAME,
+ .major = SERIAL_ATMEL_MAJOR,
+ .minor = MINOR_START,
+ .nr = ATMEL_MAX_UART,
+ .cons = ATMEL_CONSOLE_DEVICE,
};
#ifdef CONFIG_PM
-static int atmel_serial_suspend(struct platform_device *pdev, pm_message_t state)
+static int atmel_serial_suspend(struct platform_device *pdev,
+ pm_message_t state)
{
struct uart_port *port = platform_get_drvdata(pdev);
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
- if (device_may_wakeup(&pdev->dev) && !at91_suspend_entering_slow_clock())
+ if (device_may_wakeup(&pdev->dev)
+ && !at91_suspend_entering_slow_clock())
enable_irq_wake(port->irq);
else {
uart_suspend_port(&atmel_uart, port);
@@ -925,13 +971,12 @@ static int atmel_serial_suspend(struct platform_device *pdev, pm_message_t state
static int atmel_serial_resume(struct platform_device *pdev)
{
struct uart_port *port = platform_get_drvdata(pdev);
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
if (atmel_port->suspended) {
uart_resume_port(&atmel_uart, port);
atmel_port->suspended = 0;
- }
- else
+ } else
disable_irq_wake(port->irq);
return 0;
@@ -961,7 +1006,7 @@ static int __devinit atmel_serial_probe(struct platform_device *pdev)
static int __devexit atmel_serial_remove(struct platform_device *pdev)
{
struct uart_port *port = platform_get_drvdata(pdev);
- struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
int ret = 0;
clk_disable(atmel_port->clk);
--
1.5.3.8
If BRGR is zero, the baud rate generator isn't running, so the boot
loader can't have initialized the port.
Signed-off-by: Haavard Skinnemoen <[email protected]>
Acked-by: Andrew Victor <[email protected]>
---
drivers/serial/atmel_serial.c | 15 +++++++--------
1 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 4b5c6ff..746aea0 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -835,13 +835,13 @@ static void __init atmel_console_get_options(struct uart_port *port, int *baud,
{
unsigned int mr, quot;
-// TODO: CR is a write-only register
-// unsigned int cr;
-//
-// cr = UART_GET_CR(port) & (ATMEL_US_RXEN | ATMEL_US_TXEN);
-// if (cr == (ATMEL_US_RXEN | ATMEL_US_TXEN)) {
-// /* ok, the port was enabled */
-// }
+ /*
+ * If the baud rate generator isn't running, the port wasn't
+ * initialized by the boot loader.
+ */
+ quot = UART_GET_BRGR(port);
+ if (!quot)
+ return;
mr = UART_GET_MR(port) & ATMEL_US_CHRL;
if (mr == ATMEL_US_CHRL_8)
@@ -861,7 +861,6 @@ static void __init atmel_console_get_options(struct uart_port *port, int *baud,
* lower than one of those, as it would make us fall through
* to a much lower baud rate than we really want.
*/
- quot = UART_GET_BRGR(port);
*baud = port->uartclk / (16 * (quot - 1));
}
--
1.5.3.8
Replace two instances of barrier() with cpu_relax() since that's the
right thing to do when busy-waiting. This does not actually change
anything since cpu_relax() is defined as barrier() on both ARM and
AVR32.
Signed-off-by: Haavard Skinnemoen <[email protected]>
Acked-by: Andrew Victor <[email protected]>
---
drivers/serial/atmel_serial.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index ee5d844..4b5c6ff 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -616,7 +616,7 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
imr = UART_GET_IMR(port);
UART_PUT_IDR(port, -1);
while (!(UART_GET_CSR(port) & ATMEL_US_TXEMPTY))
- barrier();
+ cpu_relax();
/* disable receiver and transmitter */
UART_PUT_CR(port, ATMEL_US_TXDIS | ATMEL_US_RXDIS);
@@ -795,7 +795,7 @@ void __init atmel_register_uart_fns(struct atmel_port_fns *fns)
static void atmel_console_putchar(struct uart_port *port, int ch)
{
while (!(UART_GET_CSR(port) & ATMEL_US_TXRDY))
- barrier();
+ cpu_relax();
UART_PUT_CHAR(port, ch);
}
--
1.5.3.8
When possible, pass the tty name to request_irq() so that the user can
easily distinguish the different serial ports in /proc/interrupts.
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
drivers/serial/atmel_serial.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 2ff92b9..63505cc 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -795,6 +795,7 @@ static void atmel_tasklet_func(unsigned long data)
static int atmel_startup(struct uart_port *port)
{
struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+ struct tty_struct *tty = port->info->tty;
int retval;
/*
@@ -808,7 +809,7 @@ static int atmel_startup(struct uart_port *port)
* Allocate the IRQ
*/
retval = request_irq(port->irq, atmel_interrupt, IRQF_SHARED,
- "atmel_serial", port);
+ tty ? tty->name : "atmel_serial", port);
if (retval) {
printk("atmel_serial: atmel_startup - Can't get irq\n");
return retval;
--
1.5.3.8
From: Remy Bohmer <[email protected]>
This patch splits up the interrupt handler of the serial port
into a interrupt top-half and a tasklet.
The goal is to get the interrupt top-half as short as possible to
minimize latencies on interrupts. But the old code also does some
calls in the interrupt handler that are not allowed on preempt-RT
in IRQF_NODELAY context. This handler is executed in this context
because of the interrupt sharing with the timer interrupt.
The timer interrupt on Preempt-RT runs in IRQF_NODELAY context.
The tasklet takes care of handling control status changes, pushing
incoming characters to the tty layer, handling break and other errors.
It also handles pushing TX data into the data register.
Reading the complete receive queue is still done in the top-half
because we never want to miss any incoming character.
[[email protected]: misc cleanups and simplifications]
Signed-off-by: Remy Bohmer <[email protected]>
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
drivers/serial/atmel_serial.c | 245 +++++++++++++++++++++++++++++++---------
1 files changed, 190 insertions(+), 55 deletions(-)
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 0e715f4..0e65e98 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -104,6 +104,13 @@
static int (*atmel_open_hook)(struct uart_port *);
static void (*atmel_close_hook)(struct uart_port *);
+struct atmel_uart_char {
+ u16 status;
+ u16 ch;
+};
+
+#define ATMEL_SERIAL_RINGSIZE 1024
+
/*
* We wrap our port structure around the generic uart_port.
*/
@@ -112,6 +119,12 @@ struct atmel_uart_port {
struct clk *clk; /* uart clock */
unsigned short suspended; /* is port suspended? */
int break_active; /* break being received */
+
+ struct tasklet_struct tasklet;
+ unsigned int irq_status;
+ unsigned int irq_status_prev;
+
+ struct circ_buf rx_ring;
};
static struct atmel_uart_port atmel_ports[ATMEL_MAX_UART];
@@ -241,22 +254,42 @@ static void atmel_break_ctl(struct uart_port *port, int break_state)
}
/*
+ * Stores the incoming character in the ring buffer
+ */
+static void
+atmel_buffer_rx_char(struct uart_port *port, unsigned int status,
+ unsigned int ch)
+{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct circ_buf *ring = &atmel_port->rx_ring;
+ struct atmel_uart_char *c;
+
+ if (!CIRC_SPACE(ring->head, ring->tail, ATMEL_SERIAL_RINGSIZE))
+ /* Buffer overflow, ignore char */
+ return;
+
+ c = &((struct atmel_uart_char *)ring->buf)[ring->head];
+ c->status = status;
+ c->ch = ch;
+
+ /* Make sure the character is stored before we update head. */
+ smp_wmb();
+
+ ring->head = (ring->head + 1) & (ATMEL_SERIAL_RINGSIZE - 1);
+}
+
+/*
* Characters received (called from interrupt handler)
*/
static void atmel_rx_chars(struct uart_port *port)
{
struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
- struct tty_struct *tty = port->info->tty;
- unsigned int status, ch, flg;
+ unsigned int status, ch;
status = UART_GET_CSR(port);
while (status & ATMEL_US_RXRDY) {
ch = UART_GET_CHAR(port);
- port->icount.rx++;
-
- flg = TTY_NORMAL;
-
/*
* note that the error handling code is
* out of the main execution path
@@ -264,17 +297,14 @@ static void atmel_rx_chars(struct uart_port *port)
if (unlikely(status & (ATMEL_US_PARE | ATMEL_US_FRAME
| ATMEL_US_OVRE | ATMEL_US_RXBRK)
|| atmel_port->break_active)) {
+
/* clear error */
UART_PUT_CR(port, ATMEL_US_RSTSTA);
+
if (status & ATMEL_US_RXBRK
&& !atmel_port->break_active) {
- /* ignore side-effect */
- status &= ~(ATMEL_US_PARE | ATMEL_US_FRAME);
- port->icount.brk++;
atmel_port->break_active = 1;
UART_PUT_IER(port, ATMEL_US_RXBRK);
- if (uart_handle_break(port))
- goto ignore_char;
} else {
/*
* This is either the end-of-break
@@ -287,52 +317,30 @@ static void atmel_rx_chars(struct uart_port *port)
status &= ~ATMEL_US_RXBRK;
atmel_port->break_active = 0;
}
- if (status & ATMEL_US_PARE)
- port->icount.parity++;
- if (status & ATMEL_US_FRAME)
- port->icount.frame++;
- if (status & ATMEL_US_OVRE)
- port->icount.overrun++;
-
- status &= port->read_status_mask;
-
- if (status & ATMEL_US_RXBRK)
- flg = TTY_BREAK;
- else if (status & ATMEL_US_PARE)
- flg = TTY_PARITY;
- else if (status & ATMEL_US_FRAME)
- flg = TTY_FRAME;
}
- if (uart_handle_sysrq_char(port, ch))
- goto ignore_char;
-
- uart_insert_char(port, status, ATMEL_US_OVRE, ch, flg);
-
-ignore_char:
+ atmel_buffer_rx_char(port, status, ch);
status = UART_GET_CSR(port);
}
- tty_flip_buffer_push(tty);
+ tasklet_schedule(&atmel_port->tasklet);
}
/*
- * Transmit characters (called from interrupt handler)
+ * Transmit characters (called from tasklet with TXRDY interrupt
+ * disabled)
*/
static void atmel_tx_chars(struct uart_port *port)
{
struct circ_buf *xmit = &port->info->xmit;
- if (port->x_char) {
+ if (port->x_char && UART_GET_CSR(port) & ATMEL_US_TXRDY) {
UART_PUT_CHAR(port, port->x_char);
port->icount.tx++;
port->x_char = 0;
- return;
}
- if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
- atmel_stop_tx(port);
+ if (uart_circ_empty(xmit) || uart_tx_stopped(port))
return;
- }
while (UART_GET_CSR(port) & ATMEL_US_TXRDY) {
UART_PUT_CHAR(port, xmit->buf[xmit->tail]);
@@ -345,8 +353,8 @@ static void atmel_tx_chars(struct uart_port *port)
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(port);
- if (uart_circ_empty(xmit))
- atmel_stop_tx(port);
+ if (!uart_circ_empty(xmit))
+ UART_PUT_IER(port, ATMEL_US_TXRDY);
}
/*
@@ -372,14 +380,18 @@ atmel_handle_receive(struct uart_port *port, unsigned int pending)
}
/*
- * transmit interrupt handler.
+ * transmit interrupt handler. (Transmit is IRQF_NODELAY safe)
*/
static void
atmel_handle_transmit(struct uart_port *port, unsigned int pending)
{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+
/* Interrupt transmit */
- if (pending & ATMEL_US_TXRDY)
- atmel_tx_chars(port);
+ if (pending & ATMEL_US_TXRDY) {
+ UART_PUT_IDR(port, ATMEL_US_TXRDY);
+ tasklet_schedule(&atmel_port->tasklet);
+ }
}
/*
@@ -389,18 +401,13 @@ static void
atmel_handle_status(struct uart_port *port, unsigned int pending,
unsigned int status)
{
- /* TODO: All reads to CSR will clear these interrupts! */
- if (pending & ATMEL_US_RIIC)
- port->icount.rng++;
- if (pending & ATMEL_US_DSRIC)
- port->icount.dsr++;
- if (pending & ATMEL_US_DCDIC)
- uart_handle_dcd_change(port, !(status & ATMEL_US_DCD));
- if (pending & ATMEL_US_CTSIC)
- uart_handle_cts_change(port, !(status & ATMEL_US_CTS));
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+
if (pending & (ATMEL_US_RIIC | ATMEL_US_DSRIC | ATMEL_US_DCDIC
- | ATMEL_US_CTSIC))
- wake_up_interruptible(&port->info->delta_msr_wait);
+ | ATMEL_US_CTSIC)) {
+ atmel_port->irq_status = status;
+ tasklet_schedule(&atmel_port->tasklet);
+ }
}
/*
@@ -427,6 +434,114 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static void atmel_rx_from_ring(struct uart_port *port)
+{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct circ_buf *ring = &atmel_port->rx_ring;
+ unsigned int flg;
+ unsigned int status;
+
+ while (ring->head != ring->tail) {
+ struct atmel_uart_char c;
+
+ /* Make sure c is loaded after head. */
+ smp_rmb();
+
+ c = ((struct atmel_uart_char *)ring->buf)[ring->tail];
+
+ ring->tail = (ring->tail + 1) & (ATMEL_SERIAL_RINGSIZE - 1);
+
+ port->icount.rx++;
+ status = c.status;
+ flg = TTY_NORMAL;
+
+ /*
+ * note that the error handling code is
+ * out of the main execution path
+ */
+ if (unlikely(status & (ATMEL_US_PARE | ATMEL_US_FRAME
+ | ATMEL_US_OVRE | ATMEL_US_RXBRK))) {
+ if (status & ATMEL_US_RXBRK) {
+ /* ignore side-effect */
+ status &= ~(ATMEL_US_PARE | ATMEL_US_FRAME);
+
+ port->icount.brk++;
+ if (uart_handle_break(port))
+ continue;
+ }
+ if (status & ATMEL_US_PARE)
+ port->icount.parity++;
+ if (status & ATMEL_US_FRAME)
+ port->icount.frame++;
+ if (status & ATMEL_US_OVRE)
+ port->icount.overrun++;
+
+ status &= port->read_status_mask;
+
+ if (status & ATMEL_US_RXBRK)
+ flg = TTY_BREAK;
+ else if (status & ATMEL_US_PARE)
+ flg = TTY_PARITY;
+ else if (status & ATMEL_US_FRAME)
+ flg = TTY_FRAME;
+ }
+
+
+ if (uart_handle_sysrq_char(port, c.ch))
+ continue;
+
+ uart_insert_char(port, status, ATMEL_US_OVRE, c.ch, flg);
+ }
+
+ /*
+ * Drop the lock here since it might end up calling
+ * uart_start(), which takes the lock.
+ */
+ spin_unlock(&port->lock);
+ tty_flip_buffer_push(port->info->tty);
+ spin_lock(&port->lock);
+}
+
+/*
+ * tasklet handling tty stuff outside the interrupt handler.
+ */
+static void atmel_tasklet_func(unsigned long data)
+{
+ struct uart_port *port = (struct uart_port *)data;
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ unsigned int status;
+ unsigned int status_change;
+
+ /* The interrupt handler does not take the lock */
+ spin_lock(&port->lock);
+
+ atmel_tx_chars(port);
+
+ status = atmel_port->irq_status;
+ status_change = status ^ atmel_port->irq_status_prev;
+
+ if (status_change & (ATMEL_US_RI | ATMEL_US_DSR
+ | ATMEL_US_DCD | ATMEL_US_CTS)) {
+ /* TODO: All reads to CSR will clear these interrupts! */
+ if (status_change & ATMEL_US_RI)
+ port->icount.rng++;
+ if (status_change & ATMEL_US_DSR)
+ port->icount.dsr++;
+ if (status_change & ATMEL_US_DCD)
+ uart_handle_dcd_change(port, !(status & ATMEL_US_DCD));
+ if (status_change & ATMEL_US_CTS)
+ uart_handle_cts_change(port, !(status & ATMEL_US_CTS));
+
+ wake_up_interruptible(&port->info->delta_msr_wait);
+
+ atmel_port->irq_status_prev = status;
+ }
+
+ atmel_rx_from_ring(port);
+
+ spin_unlock(&port->lock);
+}
+
/*
* Perform initialization and enable port for reception
*/
@@ -758,6 +873,11 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
port->mapbase = pdev->resource[0].start;
port->irq = pdev->resource[1].start;
+ tasklet_init(&atmel_port->tasklet, atmel_tasklet_func,
+ (unsigned long)port);
+
+ memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
+
if (data->regs)
/* Already mapped by setup code */
port->membase = data->regs;
@@ -998,11 +1118,20 @@ static int atmel_serial_resume(struct platform_device *pdev)
static int __devinit atmel_serial_probe(struct platform_device *pdev)
{
struct atmel_uart_port *port;
+ void *data;
int ret;
+ BUILD_BUG_ON(!is_power_of_2(ATMEL_SERIAL_RINGSIZE));
+
port = &atmel_ports[pdev->id];
atmel_init_port(port, pdev);
+ ret = -ENOMEM;
+ data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
+ if (!data)
+ goto err_alloc_ring;
+ port->rx_ring.buf = data;
+
ret = uart_add_one_port(&atmel_uart, &port->uart);
if (ret)
goto err_add_port;
@@ -1013,6 +1142,9 @@ static int __devinit atmel_serial_probe(struct platform_device *pdev)
return 0;
err_add_port:
+ kfree(port->rx_ring.buf);
+ port->rx_ring.buf = NULL;
+err_alloc_ring:
if (!atmel_is_console_port(&port->uart)) {
clk_disable(port->clk);
clk_put(port->clk);
@@ -1033,6 +1165,9 @@ static int __devexit atmel_serial_remove(struct platform_device *pdev)
ret = uart_remove_one_port(&atmel_uart, port);
+ tasklet_kill(&atmel_port->tasklet);
+ kfree(atmel_port->rx_ring.buf);
+
/* "port" is allocated statically, so we shouldn't free it */
clk_disable(atmel_port->clk);
--
1.5.3.8
From: Chip Coldwell <[email protected]>
This patch is based on the DMA-patch by Chip Coldwell for the
AT91/AT32 serial USARTS, with some tweaks to make it apply neatly on
top of the other patches in this series.
The RX and TX code has been moved to a tasklet and reworked a bit.
Instead of depending on the ENDRX and TIMEOUT bits in CSR, we simply
grab as much data as we can from the DMA buffers. I think this closes
a race where the ENDRX bit is set after we read CSR but before we read
RPR, although I haven't confirmed this.
Similarly, the two TX handlers (ENDTX and TXBUFE) have been combined
into one. Since the current code only uses a single TX buffer, there's
no point in handling those interrupts separately.
This also fixes a DMA sync bug in the original patch.
[[email protected]: rebased onto irq-splitup patch]
[[email protected]: moved to tasklet, fixed dma bug, misc cleanups]
Signed-off-by: Remy Bohmer <[email protected]>
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
drivers/serial/Kconfig | 15 ++
drivers/serial/atmel_serial.c | 393 ++++++++++++++++++++++++++++++++++++++---
2 files changed, 384 insertions(+), 24 deletions(-)
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index d7e1996..67bfbb0 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -380,6 +380,21 @@ config SERIAL_ATMEL_CONSOLE
console is the device which receives all kernel messages and
warnings and which allows logins in single user mode).
+config SERIAL_ATMEL_PDC
+ bool "Support DMA transfers on AT91 / AT32 serial port"
+ depends on SERIAL_ATMEL
+ default y
+ help
+ Say Y here if you wish to use the PDC to do DMA transfers to
+ and from the Atmel AT91 / AT32 serial port. In order to
+ actually use DMA transfers, make sure that the use_dma_tx
+ and use_dma_rx members in the atmel_uart_data struct is set
+ appropriately for each port.
+
+ Note that break and error handling currently doesn't work
+ properly when DMA is enabled. Make sure that ports where
+ this matters don't use DMA.
+
config SERIAL_ATMEL_TTYAT
bool "Install as device ttyATn instead of ttySn"
depends on SERIAL_ATMEL=y
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 0e65e98..e4f9449 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -7,6 +7,8 @@
* Based on drivers/char/serial_sa1100.c, by Deep Blue Solutions Ltd.
* Based on drivers/char/serial.c, by Linus Torvalds, Theodore Ts'o.
*
+ * DMA support added by Chip Coldwell.
+ *
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
@@ -33,6 +35,7 @@
#include <linux/sysrq.h>
#include <linux/tty_flip.h>
#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
#include <linux/atmel_pdc.h>
#include <asm/io.h>
@@ -47,6 +50,10 @@
#include "atmel_serial.h"
+#define PDC_BUFFER_SIZE 512
+/* Revisit: We should calculate this based on the actual port settings */
+#define PDC_RX_TIMEOUT (3 * 10) /* 3 bytes */
+
#if defined(CONFIG_SERIAL_ATMEL_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
#define SUPPORT_SYSRQ
#endif
@@ -104,6 +111,13 @@
static int (*atmel_open_hook)(struct uart_port *);
static void (*atmel_close_hook)(struct uart_port *);
+struct atmel_dma_buffer {
+ unsigned char *buf;
+ dma_addr_t dma_addr;
+ size_t dma_size;
+ unsigned int ofs;
+};
+
struct atmel_uart_char {
u16 status;
u16 ch;
@@ -120,6 +134,13 @@ struct atmel_uart_port {
unsigned short suspended; /* is port suspended? */
int break_active; /* break being received */
+ short use_dma_rx; /* enable PDC receiver */
+ short pdc_rx_idx; /* current PDC RX buffer */
+ struct atmel_dma_buffer pdc_rx[2]; /* PDC receier */
+
+ short use_dma_tx; /* enable PDC transmitter */
+ struct atmel_dma_buffer pdc_tx; /* PDC transmitter */
+
struct tasklet_struct tasklet;
unsigned int irq_status;
unsigned int irq_status_prev;
@@ -129,10 +150,39 @@ struct atmel_uart_port {
static struct atmel_uart_port atmel_ports[ATMEL_MAX_UART];
+#define PDC_RX_BUF(port) &(port)->pdc_rx[(port)->pdc_rx_idx]
+#define PDC_RX_SWITCH(port) (port)->pdc_rx_idx = !(port)->pdc_rx_idx
+
#ifdef SUPPORT_SYSRQ
static struct console atmel_console;
#endif
+#ifdef CONFIG_SERIAL_ATMEL_PDC
+static bool atmel_use_dma_rx(struct uart_port *port)
+{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+
+ return atmel_port->use_dma_rx;
+}
+
+static bool atmel_use_dma_tx(struct uart_port *port)
+{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+
+ return atmel_port->use_dma_tx;
+}
+#else
+static bool atmel_use_dma_rx(struct uart_port *port)
+{
+ return false;
+}
+
+static bool atmel_use_dma_tx(struct uart_port *port)
+{
+ return false;
+}
+#endif
+
/*
* Return TIOCSER_TEMT when transmitter FIFO and Shift register is empty.
*/
@@ -214,7 +264,12 @@ static u_int atmel_get_mctrl(struct uart_port *port)
*/
static void atmel_stop_tx(struct uart_port *port)
{
- UART_PUT_IDR(port, ATMEL_US_TXRDY);
+ if (atmel_use_dma_tx(port)) {
+ /* disable PDC transmit */
+ UART_PUT_PTCR(port, ATMEL_PDC_TXTDIS);
+ UART_PUT_IDR(port, ATMEL_US_ENDTX | ATMEL_US_TXBUFE);
+ } else
+ UART_PUT_IDR(port, ATMEL_US_TXRDY);
}
/*
@@ -222,7 +277,17 @@ static void atmel_stop_tx(struct uart_port *port)
*/
static void atmel_start_tx(struct uart_port *port)
{
- UART_PUT_IER(port, ATMEL_US_TXRDY);
+ if (atmel_use_dma_tx(port)) {
+ if (UART_GET_PTSR(port) & ATMEL_PDC_TXTEN)
+ /* The transmitter is already running. Yes, we
+ really need this.*/
+ return;
+
+ UART_PUT_IER(port, ATMEL_US_ENDTX | ATMEL_US_TXBUFE);
+ /* re-enable PDC transmit */
+ UART_PUT_PTCR(port, ATMEL_PDC_TXTEN);
+ } else
+ UART_PUT_IER(port, ATMEL_US_TXRDY);
}
/*
@@ -230,7 +295,12 @@ static void atmel_start_tx(struct uart_port *port)
*/
static void atmel_stop_rx(struct uart_port *port)
{
- UART_PUT_IDR(port, ATMEL_US_RXRDY);
+ if (atmel_use_dma_rx(port)) {
+ /* disable PDC receive */
+ UART_PUT_PTCR(port, ATMEL_PDC_RXTDIS);
+ UART_PUT_IDR(port, ATMEL_US_ENDRX | ATMEL_US_TIMEOUT);
+ } else
+ UART_PUT_IDR(port, ATMEL_US_RXRDY);
}
/*
@@ -279,6 +349,27 @@ atmel_buffer_rx_char(struct uart_port *port, unsigned int status,
}
/*
+ * Deal with parity, framing and overrun errors.
+ */
+static void atmel_pdc_rxerr(struct uart_port *port, unsigned int status)
+{
+ /* clear error */
+ UART_PUT_CR(port, ATMEL_US_RSTSTA);
+
+ if (status & ATMEL_US_RXBRK) {
+ /* ignore side-effect */
+ status &= ~(ATMEL_US_PARE | ATMEL_US_FRAME);
+ port->icount.brk++;
+ }
+ if (status & ATMEL_US_PARE)
+ port->icount.parity++;
+ if (status & ATMEL_US_FRAME)
+ port->icount.frame++;
+ if (status & ATMEL_US_OVRE)
+ port->icount.overrun++;
+}
+
+/*
* Characters received (called from interrupt handler)
*/
static void atmel_rx_chars(struct uart_port *port)
@@ -365,6 +456,25 @@ atmel_handle_receive(struct uart_port *port, unsigned int pending)
{
struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ if (atmel_use_dma_rx(port)) {
+ /*
+ * PDC receive. Just schedule the tasklet and let it
+ * figure out the details.
+ *
+ * TODO: We're not handling error flags correctly at
+ * the moment.
+ */
+ if (pending & (ATMEL_US_ENDRX | ATMEL_US_TIMEOUT)) {
+ UART_PUT_IDR(port, (ATMEL_US_ENDRX
+ | ATMEL_US_TIMEOUT));
+ tasklet_schedule(&atmel_port->tasklet);
+ }
+
+ if (pending & (ATMEL_US_RXBRK | ATMEL_US_OVRE |
+ ATMEL_US_FRAME | ATMEL_US_PARE))
+ atmel_pdc_rxerr(port, pending);
+ }
+
/* Interrupt receive */
if (pending & ATMEL_US_RXRDY)
atmel_rx_chars(port);
@@ -387,10 +497,18 @@ atmel_handle_transmit(struct uart_port *port, unsigned int pending)
{
struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
- /* Interrupt transmit */
- if (pending & ATMEL_US_TXRDY) {
- UART_PUT_IDR(port, ATMEL_US_TXRDY);
- tasklet_schedule(&atmel_port->tasklet);
+ if (atmel_use_dma_tx(port)) {
+ /* PDC transmit */
+ if (pending & (ATMEL_US_ENDTX | ATMEL_US_TXBUFE)) {
+ UART_PUT_IDR(port, ATMEL_US_ENDTX | ATMEL_US_TXBUFE);
+ tasklet_schedule(&atmel_port->tasklet);
+ }
+ } else {
+ /* Interrupt transmit */
+ if (pending & ATMEL_US_TXRDY) {
+ UART_PUT_IDR(port, ATMEL_US_TXRDY);
+ tasklet_schedule(&atmel_port->tasklet);
+ }
}
}
@@ -418,20 +536,67 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id)
struct uart_port *port = dev_id;
unsigned int status, pending, pass_counter = 0;
- status = UART_GET_CSR(port);
- pending = status & UART_GET_IMR(port);
- while (pending) {
+ do {
+ status = UART_GET_CSR(port);
+ pending = status & UART_GET_IMR(port);
+ if (!pending)
+ break;
+
atmel_handle_receive(port, pending);
atmel_handle_status(port, pending, status);
atmel_handle_transmit(port, pending);
+ } while (pass_counter++ < ATMEL_ISR_PASS_LIMIT);
- if (pass_counter++ > ATMEL_ISR_PASS_LIMIT)
- break;
+ return IRQ_HANDLED;
+}
- status = UART_GET_CSR(port);
- pending = status & UART_GET_IMR(port);
+/*
+ * Called from tasklet with ENDTX and TXBUFE interrupts disabled.
+ */
+static void atmel_tx_dma(struct uart_port *port)
+{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct circ_buf *xmit = &port->info->xmit;
+ struct atmel_dma_buffer *pdc = &atmel_port->pdc_tx;
+ int count;
+
+ xmit->tail += pdc->ofs;
+ if (xmit->tail >= SERIAL_XMIT_SIZE)
+ xmit->tail -= SERIAL_XMIT_SIZE;
+
+ port->icount.tx += pdc->ofs;
+ pdc->ofs = 0;
+
+ if (!uart_circ_empty(xmit)) {
+ /* more to transmit - setup next transfer */
+
+ /* disable PDC transmit */
+ UART_PUT_PTCR(port, ATMEL_PDC_TXTDIS);
+ dma_sync_single_for_device(port->dev,
+ pdc->dma_addr,
+ pdc->dma_size,
+ DMA_TO_DEVICE);
+
+ if (xmit->tail < xmit->head)
+ count = xmit->head - xmit->tail;
+ else
+ count = SERIAL_XMIT_SIZE - xmit->tail;
+ pdc->ofs = count;
+
+ UART_PUT_TPR(port, pdc->dma_addr + xmit->tail);
+ UART_PUT_TCR(port, count);
+ /* re-enable PDC transmit and interrupts */
+ UART_PUT_PTCR(port, ATMEL_PDC_TXTEN);
+ UART_PUT_IER(port, ATMEL_US_ENDTX | ATMEL_US_TXBUFE);
+ } else {
+ /* nothing left to transmit - disable the transmitter */
+
+ /* disable PDC transmit */
+ UART_PUT_PTCR(port, ATMEL_PDC_TXTDIS);
}
- return IRQ_HANDLED;
+
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
}
static void atmel_rx_from_ring(struct uart_port *port)
@@ -502,6 +667,76 @@ static void atmel_rx_from_ring(struct uart_port *port)
spin_lock(&port->lock);
}
+static void atmel_rx_from_dma(struct uart_port *port)
+{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ struct tty_struct *tty = port->info->tty;
+ struct atmel_dma_buffer *pdc;
+ int rx_idx = atmel_port->pdc_rx_idx;
+ unsigned int head;
+ unsigned int tail;
+ unsigned int count;
+
+ do {
+ /* Reset the UART timeout early so that we don't miss one */
+ UART_PUT_CR(port, ATMEL_US_STTTO);
+
+ pdc = &atmel_port->pdc_rx[rx_idx];
+ head = UART_GET_RPR(port) - pdc->dma_addr;
+ tail = pdc->ofs;
+
+ /* If the PDC has switched buffers, RPR won't contain
+ * any address within the current buffer. Since head
+ * is unsigned, we just need a one-way comparison to
+ * find out.
+ *
+ * In this case, we just need to consume the entire
+ * buffer and resubmit it for DMA. This will clear the
+ * ENDRX bit as well, so that we can safely re-enable
+ * all interrupts below.
+ */
+ if (head >= pdc->dma_size)
+ head = pdc->dma_size;
+
+ if (likely(head != tail)) {
+ dma_sync_single_for_cpu(port->dev, pdc->dma_addr,
+ pdc->dma_size, DMA_FROM_DEVICE);
+
+ count = head - tail;
+ tty_insert_flip_string(tty, pdc->buf + pdc->ofs, count);
+
+ dma_sync_single_for_device(port->dev, pdc->dma_addr,
+ pdc->dma_size, DMA_FROM_DEVICE);
+
+ port->icount.rx += count;
+ pdc->ofs = head;
+ }
+
+ /*
+ * If the current buffer is full, we need to check if
+ * the next one contains any additional data.
+ */
+ if (head >= pdc->dma_size) {
+ pdc->ofs = 0;
+ UART_PUT_RNPR(port, pdc->dma_addr);
+ UART_PUT_RNCR(port, pdc->dma_size);
+
+ rx_idx = !rx_idx;
+ atmel_port->pdc_rx_idx = rx_idx;
+ }
+ } while (head >= pdc->dma_size);
+
+ /*
+ * Drop the lock here since it might end up calling
+ * uart_start(), which takes the lock.
+ */
+ spin_unlock(&port->lock);
+ tty_flip_buffer_push(tty);
+ spin_lock(&port->lock);
+
+ UART_PUT_IER(port, ATMEL_US_ENDRX | ATMEL_US_TIMEOUT);
+}
+
/*
* tasklet handling tty stuff outside the interrupt handler.
*/
@@ -515,7 +750,10 @@ static void atmel_tasklet_func(unsigned long data)
/* The interrupt handler does not take the lock */
spin_lock(&port->lock);
- atmel_tx_chars(port);
+ if (atmel_use_dma_tx(port))
+ atmel_tx_dma(port);
+ else
+ atmel_tx_chars(port);
status = atmel_port->irq_status;
status_change = status ^ atmel_port->irq_status_prev;
@@ -537,7 +775,10 @@ static void atmel_tasklet_func(unsigned long data)
atmel_port->irq_status_prev = status;
}
- atmel_rx_from_ring(port);
+ if (atmel_use_dma_rx(port))
+ atmel_rx_from_dma(port);
+ else
+ atmel_rx_from_ring(port);
spin_unlock(&port->lock);
}
@@ -547,6 +788,7 @@ static void atmel_tasklet_func(unsigned long data)
*/
static int atmel_startup(struct uart_port *port)
{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
int retval;
/*
@@ -567,6 +809,56 @@ static int atmel_startup(struct uart_port *port)
}
/*
+ * Initialize DMA (if necessary)
+ */
+ if (atmel_use_dma_rx(port)) {
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ struct atmel_dma_buffer *pdc = &atmel_port->pdc_rx[i];
+
+ pdc->buf = kmalloc(PDC_BUFFER_SIZE, GFP_KERNEL);
+ if (pdc->buf == NULL) {
+ if (i != 0) {
+ dma_unmap_single(port->dev,
+ atmel_port->pdc_rx[0].dma_addr,
+ PDC_BUFFER_SIZE,
+ DMA_FROM_DEVICE);
+ kfree(atmel_port->pdc_rx[0].buf);
+ }
+ free_irq(port->irq, port);
+ return -ENOMEM;
+ }
+ pdc->dma_addr = dma_map_single(port->dev,
+ pdc->buf,
+ PDC_BUFFER_SIZE,
+ DMA_FROM_DEVICE);
+ pdc->dma_size = PDC_BUFFER_SIZE;
+ pdc->ofs = 0;
+ }
+
+ atmel_port->pdc_rx_idx = 0;
+
+ UART_PUT_RPR(port, atmel_port->pdc_rx[0].dma_addr);
+ UART_PUT_RCR(port, PDC_BUFFER_SIZE);
+
+ UART_PUT_RNPR(port, atmel_port->pdc_rx[1].dma_addr);
+ UART_PUT_RNCR(port, PDC_BUFFER_SIZE);
+ }
+ if (atmel_use_dma_tx(port)) {
+ struct atmel_dma_buffer *pdc = &atmel_port->pdc_tx;
+ struct circ_buf *xmit = &port->info->xmit;
+
+ pdc->buf = xmit->buf;
+ pdc->dma_addr = dma_map_single(port->dev,
+ pdc->buf,
+ SERIAL_XMIT_SIZE,
+ DMA_TO_DEVICE);
+ pdc->dma_size = SERIAL_XMIT_SIZE;
+ pdc->ofs = 0;
+ }
+
+ /*
* If there is a specific "open" function (to register
* control line interrupts)
*/
@@ -585,8 +877,18 @@ static int atmel_startup(struct uart_port *port)
/* enable xmit & rcvr */
UART_PUT_CR(port, ATMEL_US_TXEN | ATMEL_US_RXEN);
- /* enable receive only */
- UART_PUT_IER(port, ATMEL_US_RXRDY);
+ if (atmel_use_dma_rx(port)) {
+ /* set UART timeout */
+ UART_PUT_RTOR(port, PDC_RX_TIMEOUT);
+ UART_PUT_CR(port, ATMEL_US_STTTO);
+
+ UART_PUT_IER(port, ATMEL_US_ENDRX | ATMEL_US_TIMEOUT);
+ /* enable PDC controller */
+ UART_PUT_PTCR(port, ATMEL_PDC_RXTEN);
+ } else {
+ /* enable receive only */
+ UART_PUT_IER(port, ATMEL_US_RXRDY);
+ }
return 0;
}
@@ -596,6 +898,38 @@ static int atmel_startup(struct uart_port *port)
*/
static void atmel_shutdown(struct uart_port *port)
{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+ /*
+ * Ensure everything is stopped.
+ */
+ atmel_stop_rx(port);
+ atmel_stop_tx(port);
+
+ /*
+ * Shut-down the DMA.
+ */
+ if (atmel_use_dma_rx(port)) {
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ struct atmel_dma_buffer *pdc = &atmel_port->pdc_rx[i];
+
+ dma_unmap_single(port->dev,
+ pdc->dma_addr,
+ pdc->dma_size,
+ DMA_FROM_DEVICE);
+ kfree(pdc->buf);
+ }
+ }
+ if (atmel_use_dma_tx(port)) {
+ struct atmel_dma_buffer *pdc = &atmel_port->pdc_tx;
+
+ dma_unmap_single(port->dev,
+ pdc->dma_addr,
+ pdc->dma_size,
+ DMA_TO_DEVICE);
+ }
+
/*
* Disable all interrupts, port and break condition.
*/
@@ -707,6 +1041,10 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
if (termios->c_iflag & (BRKINT | PARMRK))
port->read_status_mask |= ATMEL_US_RXBRK;
+ if (atmel_use_dma_rx(port))
+ /* need to enable error interrupts */
+ UART_PUT_IER(port, port->read_status_mask);
+
/*
* Characters to ignore
*/
@@ -892,6 +1230,11 @@ static void __devinit atmel_init_port(struct atmel_uart_port *atmel_port,
clk_enable(atmel_port->clk);
port->uartclk = clk_get_rate(atmel_port->clk);
}
+
+ atmel_port->use_dma_rx = data->use_dma_rx;
+ atmel_port->use_dma_tx = data->use_dma_tx;
+ if (atmel_use_dma_tx(port))
+ port->fifosize = PDC_BUFFER_SIZE;
}
/*
@@ -1126,11 +1469,13 @@ static int __devinit atmel_serial_probe(struct platform_device *pdev)
port = &atmel_ports[pdev->id];
atmel_init_port(port, pdev);
- ret = -ENOMEM;
- data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
- if (!data)
- goto err_alloc_ring;
- port->rx_ring.buf = data;
+ if (!atmel_use_dma_rx(&port->uart)) {
+ ret = -ENOMEM;
+ data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
+ if (!data)
+ goto err_alloc_ring;
+ port->rx_ring.buf = data;
+ }
ret = uart_add_one_port(&atmel_uart, &port->uart);
if (ret)
--
1.5.3.8
Tested and working on at91rm9200 using 2.6.24-rc8, in one word... ack.
Regards
Marc
On Thu, 24 Jan 2008 14:32:48 +0100
Marc Pignat <[email protected]> wrote:
> Tested and working on at91rm9200 using 2.6.24-rc8, in one word... ack.
Great! Thanks for testing!
Haavard
> On Thu, 24 Jan 2008 13:41:49 +0100 Haavard Skinnemoen <[email protected]> wrote:
> From: Chip Coldwell <[email protected]>
>
> This patch is based on the DMA-patch by Chip Coldwell for the
> AT91/AT32 serial USARTS, with some tweaks to make it apply neatly on
> top of the other patches in this series.
>
> The RX and TX code has been moved to a tasklet and reworked a bit.
> Instead of depending on the ENDRX and TIMEOUT bits in CSR, we simply
> grab as much data as we can from the DMA buffers. I think this closes
> a race where the ENDRX bit is set after we read CSR but before we read
> RPR, although I haven't confirmed this.
>
> Similarly, the two TX handlers (ENDTX and TXBUFE) have been combined
> into one. Since the current code only uses a single TX buffer, there's
> no point in handling those interrupts separately.
>
> This also fixes a DMA sync bug in the original patch.
>
> ...
>
> +#define PDC_RX_BUF(port) &(port)->pdc_rx[(port)->pdc_rx_idx]
> +#define PDC_RX_SWITCH(port) (port)->pdc_rx_idx = !(port)->pdc_rx_idx
These macros refer to their arg more than one time and hance are dangerous.
Think of the effects of PDC_RX_BUF(foo++).
Generally, please don't use macros for anything which can be coded as a
regular C function.
> +/*
> + * Called from tasklet with ENDTX and TXBUFE interrupts disabled.
> + */
> +static void atmel_tx_dma(struct uart_port *port)
> +{
> + struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
> + struct circ_buf *xmit = &port->info->xmit;
> + struct atmel_dma_buffer *pdc = &atmel_port->pdc_tx;
> + int count;
> +
> + xmit->tail += pdc->ofs;
> + if (xmit->tail >= SERIAL_XMIT_SIZE)
> + xmit->tail -= SERIAL_XMIT_SIZE;
Maybe this should be a uart_circ_whatever() helper rather than open-coded.
> + port->icount.tx += pdc->ofs;
> + pdc->ofs = 0;
> +
> + if (!uart_circ_empty(xmit)) {
ho-hum. The generic uart buffer-handling code does ringbuffers the wrong
way. Maybe it has to handle non-power-of-two buffer sizes.
> + /* more to transmit - setup next transfer */
> +
> + /* disable PDC transmit */
> + UART_PUT_PTCR(port, ATMEL_PDC_TXTDIS);
> + dma_sync_single_for_device(port->dev,
> + pdc->dma_addr,
> + pdc->dma_size,
> + DMA_TO_DEVICE);
> +
> + if (xmit->tail < xmit->head)
> + count = xmit->head - xmit->tail;
> + else
> + count = SERIAL_XMIT_SIZE - xmit->tail;
Doesn't uart_circ_chars_pending() do this?
All those uart_circ_*() macros reference their arg more than once and ...
you know the deal.
> + pdc->ofs = count;
> +
> + UART_PUT_TPR(port, pdc->dma_addr + xmit->tail);
> + UART_PUT_TCR(port, count);
> + /* re-enable PDC transmit and interrupts */
> + UART_PUT_PTCR(port, ATMEL_PDC_TXTEN);
> + UART_PUT_IER(port, ATMEL_US_ENDTX | ATMEL_US_TXBUFE);
> + } else {
> + /* nothing left to transmit - disable the transmitter */
> +
> + /* disable PDC transmit */
> + UART_PUT_PTCR(port, ATMEL_PDC_TXTDIS);
> }
> - return IRQ_HANDLED;
> +
> + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> + uart_write_wakeup(port);
> }
>
> static void atmel_rx_from_ring(struct uart_port *port)
> @@ -502,6 +667,76 @@ static void atmel_rx_from_ring(struct uart_port *port)
> spin_lock(&port->lock);
> }
>
> +static void atmel_rx_from_dma(struct uart_port *port)
> +{
> + struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
> + struct tty_struct *tty = port->info->tty;
> + struct atmel_dma_buffer *pdc;
> + int rx_idx = atmel_port->pdc_rx_idx;
> + unsigned int head;
> + unsigned int tail;
> + unsigned int count;
> +
> + do {
> + /* Reset the UART timeout early so that we don't miss one */
> + UART_PUT_CR(port, ATMEL_US_STTTO);
> +
> + pdc = &atmel_port->pdc_rx[rx_idx];
> + head = UART_GET_RPR(port) - pdc->dma_addr;
> + tail = pdc->ofs;
> +
> + /* If the PDC has switched buffers, RPR won't contain
> + * any address within the current buffer. Since head
> + * is unsigned, we just need a one-way comparison to
> + * find out.
> + *
> + * In this case, we just need to consume the entire
> + * buffer and resubmit it for DMA. This will clear the
> + * ENDRX bit as well, so that we can safely re-enable
> + * all interrupts below.
> + */
> + if (head >= pdc->dma_size)
> + head = pdc->dma_size;
min()?
> + if (likely(head != tail)) {
> + dma_sync_single_for_cpu(port->dev, pdc->dma_addr,
> + pdc->dma_size, DMA_FROM_DEVICE);
> +
> + count = head - tail;
No wraparound issues here?
> + tty_insert_flip_string(tty, pdc->buf + pdc->ofs, count);
> +
> + dma_sync_single_for_device(port->dev, pdc->dma_addr,
> + pdc->dma_size, DMA_FROM_DEVICE);
> +
> + port->icount.rx += count;
> + pdc->ofs = head;
> + }
> +
> + /*
> + * If the current buffer is full, we need to check if
> + * the next one contains any additional data.
> + */
> + if (head >= pdc->dma_size) {
> + pdc->ofs = 0;
> + UART_PUT_RNPR(port, pdc->dma_addr);
> + UART_PUT_RNCR(port, pdc->dma_size);
> +
> + rx_idx = !rx_idx;
> + atmel_port->pdc_rx_idx = rx_idx;
> + }
> + } while (head >= pdc->dma_size);
> +
> + /*
> + * Drop the lock here since it might end up calling
> + * uart_start(), which takes the lock.
> + */
> + spin_unlock(&port->lock);
> + tty_flip_buffer_push(tty);
> + spin_lock(&port->lock);
> +
> + UART_PUT_IER(port, ATMEL_US_ENDRX | ATMEL_US_TIMEOUT);
> +}
> +
On Sat, 26 Jan 2008 22:02:00 -0800
Andrew Morton <[email protected]> wrote:
> > On Thu, 24 Jan 2008 13:41:49 +0100 Haavard Skinnemoen <[email protected]> wrote:
> > +#define PDC_RX_BUF(port) &(port)->pdc_rx[(port)->pdc_rx_idx]
> > +#define PDC_RX_SWITCH(port) (port)->pdc_rx_idx = !(port)->pdc_rx_idx
>
> These macros refer to their arg more than one time and hance are dangerous.
> Think of the effects of PDC_RX_BUF(foo++).
They're also unused...I'll remove them.
> Generally, please don't use macros for anything which can be coded as a
> regular C function.
Agree.
> > +/*
> > + * Called from tasklet with ENDTX and TXBUFE interrupts disabled.
> > + */
> > +static void atmel_tx_dma(struct uart_port *port)
> > +{
> > + struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
> > + struct circ_buf *xmit = &port->info->xmit;
> > + struct atmel_dma_buffer *pdc = &atmel_port->pdc_tx;
> > + int count;
> > +
> > + xmit->tail += pdc->ofs;
> > + if (xmit->tail >= SERIAL_XMIT_SIZE)
> > + xmit->tail -= SERIAL_XMIT_SIZE;
>
> Maybe this should be a uart_circ_whatever() helper rather than open-coded.
Hmm. uart_circ_get_multiple() or something?
Also, we should probably be using UART_XMIT_SIZE here, not
SERIAL_XMIT_SIZE. Why do we have two such definitions (which both
happen to be PAGE_SIZE)?
> > + port->icount.tx += pdc->ofs;
> > + pdc->ofs = 0;
> > +
> > + if (!uart_circ_empty(xmit)) {
>
> ho-hum. The generic uart buffer-handling code does ringbuffers the wrong
> way. Maybe it has to handle non-power-of-two buffer sizes.
Hmm...I don't understand. What does it do wrong?
> > + /* more to transmit - setup next transfer */
> > +
> > + /* disable PDC transmit */
> > + UART_PUT_PTCR(port, ATMEL_PDC_TXTDIS);
> > + dma_sync_single_for_device(port->dev,
> > + pdc->dma_addr,
> > + pdc->dma_size,
> > + DMA_TO_DEVICE);
> > +
> > + if (xmit->tail < xmit->head)
> > + count = xmit->head - xmit->tail;
> > + else
> > + count = SERIAL_XMIT_SIZE - xmit->tail;
>
> Doesn't uart_circ_chars_pending() do this?
Hmm...no. I think we really want something CIRC_CNT_TO_END()-ish.
> All those uart_circ_*() macros reference their arg more than once and ...
> you know the deal.
Yeah. Would you like a patch that inline-ifies <linux/circ.h>?
> > + if (head >= pdc->dma_size)
> > + head = pdc->dma_size;
>
> min()?
Absolutely.
> > + if (likely(head != tail)) {
> > + dma_sync_single_for_cpu(port->dev, pdc->dma_addr,
> > + pdc->dma_size, DMA_FROM_DEVICE);
> > +
> > + count = head - tail;
>
> No wraparound issues here?
No. "head" is the current offset that the DMA controller is reading
from, while "tail" is the offset we stopped at the last time around.
head will only wrap around when we recycle the DMA buffer, and when
that happens, we explicitly set tail to 0.
Haavard
On Mon, 28 Jan 2008 10:59:09 +0100 Haavard Skinnemoen <[email protected]> wrote:
> > ho-hum. The generic uart buffer-handling code does ringbuffers the wrong
> > way. Maybe it has to handle non-power-of-two buffer sizes.
>
> Hmm...I don't understand. What does it do wrong?
An faq ;) If the buffer size is a power-of-two it's better to allow the
head and tail indices wrap through 0xffffffff and only mask them when
subscripting. It ends up faster (usually) and you can use all of the
elements of the buffer (rather than all-1) and you get nice things like:
is_empty = (head == tail)
is_full = (tail - head == size)
nr_items_in_ring = (tail - head)
> > > + /* more to transmit - setup next transfer */
> > > +
> > > + /* disable PDC transmit */
> > > + UART_PUT_PTCR(port, ATMEL_PDC_TXTDIS);
> > > + dma_sync_single_for_device(port->dev,
> > > + pdc->dma_addr,
> > > + pdc->dma_size,
> > > + DMA_TO_DEVICE);
> > > +
> > > + if (xmit->tail < xmit->head)
> > > + count = xmit->head - xmit->tail;
> > > + else
> > > + count = SERIAL_XMIT_SIZE - xmit->tail;
> >
> > Doesn't uart_circ_chars_pending() do this?
>
> Hmm...no. I think we really want something CIRC_CNT_TO_END()-ish.
>
> > All those uart_circ_*() macros reference their arg more than once and ...
> > you know the deal.
>
> Yeah. Would you like a patch that inline-ifies <linux/circ.h>?
uh, if you're feeling especially keen. We have bigger problems than this.
On Mon, 28 Jan 2008 02:20:00 -0800
Andrew Morton <[email protected]> wrote:
> On Mon, 28 Jan 2008 10:59:09 +0100 Haavard Skinnemoen <[email protected]> wrote:
>
> > > ho-hum. The generic uart buffer-handling code does ringbuffers the wrong
> > > way. Maybe it has to handle non-power-of-two buffer sizes.
> >
> > Hmm...I don't understand. What does it do wrong?
>
> An faq ;) If the buffer size is a power-of-two it's better to allow the
> head and tail indices wrap through 0xffffffff and only mask them when
> subscripting. It ends up faster (usually) and you can use all of the
> elements of the buffer (rather than all-1) and you get nice things like:
>
> is_empty = (head == tail)
> is_full = (tail - head == size)
> nr_items_in_ring = (tail - head)
Ok, that makes sense. Thanks for explaining. Not sure if want to start
improving things right now, although I'm pretty sure the circ stuff
can't handle non-power-of-two buffer size currently so it should be
possible.
> > > All those uart_circ_*() macros reference their arg more than once and ...
> > > you know the deal.
> >
> > Yeah. Would you like a patch that inline-ifies <linux/circ.h>?
>
> uh, if you're feeling especially keen. We have bigger problems than this.
Well, if you put it like that; no, not really.
I'll post a fix for the other things you pointed out shortly.
Haavard
Fix some issues identified by Andrew Morton:
* Kill unused PDC_RX_BUF() and PDC_RX_SWITCH() macros
* Simplify TX circ buffer management a bit.
* Don't open-code min()
* Add comment explaining why "count = head - tail" will not cause
wraparound problems.
Also, s/SERIAL_XMIT_SIZE/UART_XMIT_SIZE/ since the latter is what the
serial core circ macros use.
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
Please fold into atmel_serial-add-dma-support.patch, or let me know if
you want a replacement for that patch.
drivers/serial/atmel_serial.c | 27 +++++++++++++--------------
1 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 63505cc..62654ab 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -114,7 +114,7 @@ static void (*atmel_close_hook)(struct uart_port *);
struct atmel_dma_buffer {
unsigned char *buf;
dma_addr_t dma_addr;
- size_t dma_size;
+ unsigned int dma_size;
unsigned int ofs;
};
@@ -150,9 +150,6 @@ struct atmel_uart_port {
static struct atmel_uart_port atmel_ports[ATMEL_MAX_UART];
-#define PDC_RX_BUF(port) &(port)->pdc_rx[(port)->pdc_rx_idx]
-#define PDC_RX_SWITCH(port) (port)->pdc_rx_idx = !(port)->pdc_rx_idx
-
#ifdef SUPPORT_SYSRQ
static struct console atmel_console;
#endif
@@ -567,8 +564,7 @@ static void atmel_tx_dma(struct uart_port *port)
int count;
xmit->tail += pdc->ofs;
- if (xmit->tail >= SERIAL_XMIT_SIZE)
- xmit->tail -= SERIAL_XMIT_SIZE;
+ xmit->tail &= UART_XMIT_SIZE - 1;
port->icount.tx += pdc->ofs;
pdc->ofs = 0;
@@ -583,10 +579,7 @@ static void atmel_tx_dma(struct uart_port *port)
pdc->dma_size,
DMA_TO_DEVICE);
- if (xmit->tail < xmit->head)
- count = xmit->head - xmit->tail;
- else
- count = SERIAL_XMIT_SIZE - xmit->tail;
+ count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
pdc->ofs = count;
UART_PUT_TPR(port, pdc->dma_addr + xmit->tail);
@@ -701,14 +694,20 @@ static void atmel_rx_from_dma(struct uart_port *port)
* ENDRX bit as well, so that we can safely re-enable
* all interrupts below.
*/
- if (head >= pdc->dma_size)
- head = pdc->dma_size;
+ head = min(head, pdc->dma_size);
if (likely(head != tail)) {
dma_sync_single_for_cpu(port->dev, pdc->dma_addr,
pdc->dma_size, DMA_FROM_DEVICE);
+ /*
+ * head will only wrap around when we recycle
+ * the DMA buffer, and when that happens, we
+ * explicitly set tail to 0. So head will
+ * always be greater than tail.
+ */
count = head - tail;
+
tty_insert_flip_string(tty, pdc->buf + pdc->ofs, count);
dma_sync_single_for_device(port->dev, pdc->dma_addr,
@@ -859,9 +858,9 @@ static int atmel_startup(struct uart_port *port)
pdc->buf = xmit->buf;
pdc->dma_addr = dma_map_single(port->dev,
pdc->buf,
- SERIAL_XMIT_SIZE,
+ UART_XMIT_SIZE,
DMA_TO_DEVICE);
- pdc->dma_size = SERIAL_XMIT_SIZE;
+ pdc->dma_size = UART_XMIT_SIZE;
pdc->ofs = 0;
}
--
1.5.3.8
Hi,
>> From: Remy Bohmer <[email protected]>
>>
>> This patch splits up the interrupt handler of the serial port
>> into a interrupt top-half and a tasklet.
>>
>> The goal is to get the interrupt top-half as short as possible to
>> minimize latencies on interrupts. But the old code also does some
>> calls in the interrupt handler that are not allowed on preempt-RT
>> in IRQF_NODELAY context. This handler is executed in this context
>> because of the interrupt sharing with the timer interrupt.
>> +static void
>> +atmel_buffer_rx_char(struct uart_port *port, unsigned int status,
>> + unsigned int ch)
>> +{
>> + struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
>> + struct circ_buf *ring = &atmel_port->rx_ring;
>> + struct atmel_uart_char *c;
>>
I'm testing this patch on an at91sam9260 on 2.6.24-rt. I'm using this
patch with the tclib support for hrtimer and the clocksource pit_clk.
These are the results:
- Voluntary Kernel Preemption the system (crash)
- Preemptible Kernel (crash)
/*
* Drop the lock here since it might end up calling
* uart_start(), which takes the lock.
spin_unlock(&port->lock);
*/
tty_flip_buffer_push(port->info->tty);
/*
spin_lock(&port->lock);
*/
The same code with this comments out runs
Complete Preemption (Real-Time) ok but the serials is just unusable due
to too many overruns (just using lrz)
The system is stable and doesn't crash reverting this patch.
I don't test with the thread hardirqs active.
>> +
>> + if (!CIRC_SPACE(ring->head, ring->tail, ATMEL_SERIAL_RINGSIZE))
>> + /* Buffer overflow, ignore char */
>> + return;
>> +
>> + c = &((struct atmel_uart_char *)ring->buf)[ring->head];
>> + c->status = status;
>> + c->ch = ch;
>> +
>> + /* Make sure the character is stored before we update head. */
>> + smp_wmb();
>> +
>> + ring->head = (ring->head + 1) & (ATMEL_SERIAL_RINGSIZE - 1);
>> +}
>> +
>>
...
>> +
>> port = &atmel_ports[pdev->id];
>> atmel_init_port(port, pdev);
>>
>> + ret = -ENOMEM;
>> + data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
>> + if (!data)
>> + goto err_alloc_ring;
>> + port->rx_ring.buf = data;
>> +
>> ret = uart_add_one_port(&atmel_uart, &port->uart);
>>
Is the kmalloc correct?
maybe:
data = kmalloc(ATMEL_SERIAL_RINGSIZE * sizeof(struct atmel_uart_char),
GFP_KERNEL);
>> if (ret)
>> goto err_add_port;
>> @@ -1013,6 +1142,9 @@ static int __devinit atmel_serial_probe(struct platform_device *pdev)
>> return 0;
>>
>> err_add_port:
>> + kfree(port->rx_ring.buf);
>> + port->rx_ring.buf = NULL;
>> +err_alloc_ring:
>> if (!atmel_is_console_port(&port->uart)) {
>> clk_disable(port->clk);
>> clk_put(port->clk);
>> @@ -1033,6 +1165,9 @@ static int __devexit atmel_serial_remove(struct platform_device *pdev)
>>
>> ret = uart_remove_one_port(&atmel_uart, port);
>>
>> + tasklet_kill(&atmel_port->tasklet);
>> + kfree(atmel_port->rx_ring.buf);
>> +
>>
Why the tasklet_kill is not done in atmel_shutdown?
Regards
Michael
On Wed, 30 Jan 2008 00:12:23 +0100
michael <[email protected]> wrote:
> I'm testing this patch on an at91sam9260 on 2.6.24-rt. I'm using this
> patch with the tclib support for hrtimer and the clocksource pit_clk.
> These are the results:
>
> - Voluntary Kernel Preemption the system (crash)
> - Preemptible Kernel (crash)
Ouch. I'm assuming this is with DMA disabled?
> /*
> * Drop the lock here since it might end up calling
> * uart_start(), which takes the lock.
> spin_unlock(&port->lock);
> */
> tty_flip_buffer_push(port->info->tty);
> /*
> spin_lock(&port->lock);
> */
> The same code with this comments out runs
Now, _that_ is strange. I can't see anything that needs protection
across that call; in fact, I think we can lock a lot less than what we
currently do.
> Complete Preemption (Real-Time) ok but the serials is just unusable due
> to too many overruns (just using lrz)
Is it worse than before? IIRC Remy mentioned something about
IRQF_NODELAY being the reason for moving all this code to softirq
context in the first place; does the interrupt handler run in hardirq
context?
> The system is stable and doesn't crash reverting this patch.
> I don't test with the thread hardirqs active.
Ok.
> >> + ret = -ENOMEM;
> >> + data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
> >> + if (!data)
> >> + goto err_alloc_ring;
> >> + port->rx_ring.buf = data;
> >> +
> >> ret = uart_add_one_port(&atmel_uart, &port->uart);
> >>
> Is the kmalloc correct?
> maybe:
> data = kmalloc(ATMEL_SERIAL_RINGSIZE * sizeof(struct atmel_uart_char),
> GFP_KERNEL);
I think you're right. Can you change it and see if it helps?
I guess I didn't test it thoroughly enough with DMA
disabled...slub_debug ought to catch such things, but not until we
receive enough data to actually overflow the buffer.
> >> @@ -1033,6 +1165,9 @@ static int __devexit atmel_serial_remove(struct platform_device *pdev)
> >>
> >> ret = uart_remove_one_port(&atmel_uart, port);
> >>
> >> + tasklet_kill(&atmel_port->tasklet);
> >> + kfree(atmel_port->rx_ring.buf);
> >> +
> >>
> Why the tasklet_kill is not done in atmel_shutdown?
Why should it be? If it should, we must move the call to tasklet_init
into atmel_startup too, and I don't really see the point.
Haavard
Hello Haavard (and Michael),
First I want to mention that I did not found the time yet to test your
latest integration of these atmel_serial patches, and I only know that
your set of the end of December works fine. I do not know the changes
you made since posting that set and your latest patch-set.
But let me clear some things up:
The original patchset I posted, existed of a set of patches suitable
for the mainline kernel, plus an additional patch for Preempt-RT only.
So, the splitup of the interrupt handler was also needed for
Preempt-RT, but it was not the only patch that was needed on
preempt-rt.
Now looking at this problem:
> > * Drop the lock here since it might end up calling
> > * uart_start(), which takes the lock.
> > spin_unlock(&port->lock);
> > */
> > tty_flip_buffer_push(port->info->tty);
> > /*
> > spin_lock(&port->lock);
> > */
> > The same code with this comments out runs
I expect the UART generating the problem is the DBGU port. The DBGU
shares its interrupt line with the timer interrupt with the IRQF_TIMER
flag set, and thus the DBGU interrupt handler is running in
IRQF_NODELAY context. Within this context it is forbidden to lock a
normal spinlock, because a normal spinlock is converted to a mutex on
Preempt-RT; a mutex can sleep which is forbidden in interrupt context.
So, to get around this problem, this lock spinlock has to be of the
raw_spinlock_t type. The raw_spinlock_t is the normal mainline-kernel
spinlock, and as such it is not converted to a mutex, and will
therefor never sleep.
Attached a patch that changes this spinlock type. I used it in my
patchset, but your updates of December last year do not need this
patch anymore, so apparantly you changed something that has a
regression on Preempt-RT...
> > Complete Preemption (Real-Time) ok but the serials is just unusable due
> > to too many overruns (just using lrz)
This problem is not there in the December-set either. It works like a charm...
I believe I have to look at the latest set of patches, and try to find
any regressions. Do you have a location somewhere where I can download
the latest versions? Or do I need to dig through LKML to find the
latest... ;-)
Kind Regards,
Remy
2008/1/30, Haavard Skinnemoen <[email protected]>:
> On Wed, 30 Jan 2008 00:12:23 +0100
> michael <[email protected]> wrote:
>
> > I'm testing this patch on an at91sam9260 on 2.6.24-rt. I'm using this
> > patch with the tclib support for hrtimer and the clocksource pit_clk.
> > These are the results:
> >
> > - Voluntary Kernel Preemption the system (crash)
> > - Preemptible Kernel (crash)
>
> Ouch. I'm assuming this is with DMA disabled?
>
> > /*
> > * Drop the lock here since it might end up calling
> > * uart_start(), which takes the lock.
> > spin_unlock(&port->lock);
> > */
> > tty_flip_buffer_push(port->info->tty);
> > /*
> > spin_lock(&port->lock);
> > */
> > The same code with this comments out runs
>
> Now, _that_ is strange. I can't see anything that needs protection
> across that call; in fact, I think we can lock a lot less than what we
> currently do.
>
> > Complete Preemption (Real-Time) ok but the serials is just unusable due
> > to too many overruns (just using lrz)
>
> Is it worse than before? IIRC Remy mentioned something about
> IRQF_NODELAY being the reason for moving all this code to softirq
> context in the first place; does the interrupt handler run in hardirq
> context?
>
> > The system is stable and doesn't crash reverting this patch.
> > I don't test with the thread hardirqs active.
>
> Ok.
>
> > >> + ret = -ENOMEM;
> > >> + data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
> > >> + if (!data)
> > >> + goto err_alloc_ring;
> > >> + port->rx_ring.buf = data;
> > >> +
> > >> ret = uart_add_one_port(&atmel_uart, &port->uart);
> > >>
> > Is the kmalloc correct?
> > maybe:
> > data = kmalloc(ATMEL_SERIAL_RINGSIZE * sizeof(struct atmel_uart_char),
> > GFP_KERNEL);
>
> I think you're right. Can you change it and see if it helps?
>
> I guess I didn't test it thoroughly enough with DMA
> disabled...slub_debug ought to catch such things, but not until we
> receive enough data to actually overflow the buffer.
>
> > >> @@ -1033,6 +1165,9 @@ static int __devexit atmel_serial_remove(struct platform_device *pdev)
> > >>
> > >> ret = uart_remove_one_port(&atmel_uart, port);
> > >>
> > >> + tasklet_kill(&atmel_port->tasklet);
> > >> + kfree(atmel_port->rx_ring.buf);
> > >> +
> > >>
> > Why the tasklet_kill is not done in atmel_shutdown?
>
> Why should it be? If it should, we must move the call to tasklet_init
> into atmel_startup too, and I don't really see the point.
>
> Haavard
>
Hi
> On Wed, 30 Jan 2008 00:12:23 +0100
> michael <[email protected]> wrote:
>
>
>> - Voluntary Kernel Preemption the system (crash)
>> - Preemptible Kernel (crash)
>>
>
> Ouch. I'm assuming this is with DMA disabled?
>
>
Yes, is with DMA disabled
>> /*
>> * Drop the lock here since it might end up calling
>> * uart_start(), which takes the lock.
>> spin_unlock(&port->lock);
>> */
>> tty_flip_buffer_push(port->info->tty);
>> /*
>> spin_lock(&port->lock);
>> */
>> The same code with this comments out runs
>>
>
> Now, _that_ is strange. I can't see anything that needs protection
> across that call; in fact, I think we can lock a lot less than what we
> currently do.
>
>
I explain it bad:
- with spin_lock the system seems, there is no problem with Valuntary
Preeption and
Preemptible Kernel
- with full preemption it runs but the serial line can't be used for
receiving at
high bit rate (using lrz)
>> Complete Preemption (Real-Time) ok but the serials is just unusable due
>> to too many overruns (just using lrz)
>>
>
> Is it worse than before? IIRC Remy mentioned something about
> IRQF_NODELAY being the reason for moving all this code to softirq
> context in the first place; does the interrupt handler run in hardirq
> context?
>
>
In the complete preemption yes.
>> The system is stable and doesn't crash reverting this patch.
>> I don't test with the thread hardirqs active.
>>
>
> Ok.
>
>
>> Is the kmalloc correct?
>> maybe:
>> data = kmalloc(ATMEL_SERIAL_RINGSIZE * sizeof(struct atmel_uart_char),
>> GFP_KERNEL);
>>
>
> I think you're right. Can you change it and see if it helps?
>
>
I just change it because I have corruption on receiving buffer. All
my test are done with this fix
> I guess I didn't test it thoroughly enough with DMA
> disabled...slub_debug ought to catch such things, but not until we
> receive enough data to actually overflow the buffer.
>
>
I just test it I don't have
buffer overflow.
I protect with a spinlock the access to the register when we sending
from the tasklet. It is correct?
>
> Why should it be? If it should, we must move the call to tasklet_init
> into atmel_startup too, and I don't really see the point.
>
>
Ok
Regards Michael
On Wed, 30 Jan 2008 11:21:49 +0100
"Remy Bohmer" <[email protected]> wrote:
> > > * Drop the lock here since it might end up calling
> > > * uart_start(), which takes the lock.
> > > spin_unlock(&port->lock);
> > > */
> > > tty_flip_buffer_push(port->info->tty);
> > > /*
> > > spin_lock(&port->lock);
> > > */
> > > The same code with this comments out runs
>
> I expect the UART generating the problem is the DBGU port. The DBGU
> shares its interrupt line with the timer interrupt with the IRQF_TIMER
> flag set, and thus the DBGU interrupt handler is running in
> IRQF_NODELAY context. Within this context it is forbidden to lock a
> normal spinlock, because a normal spinlock is converted to a mutex on
> Preempt-RT; a mutex can sleep which is forbidden in interrupt context.
> So, to get around this problem, this lock spinlock has to be of the
> raw_spinlock_t type. The raw_spinlock_t is the normal mainline-kernel
> spinlock, and as such it is not converted to a mutex, and will
> therefor never sleep.
>
> Attached a patch that changes this spinlock type. I used it in my
> patchset, but your updates of December last year do not need this
> patch anymore, so apparantly you changed something that has a
> regression on Preempt-RT...
The code above is from the tasklet, so I don't think that's the problem.
The interrupt handler doesn't take any locks.
Or are spinlocks not allowed in softirq context either?
> I believe I have to look at the latest set of patches, and try to find
> any regressions. Do you have a location somewhere where I can download
> the latest versions? Or do I need to dig through LKML to find the
> latest... ;-)
They are in -mm. You were Cc'ed I think...
Haavard
Haavard
Hello Haavard,
> The code above is from the tasklet, so I don't think that's the problem.
> The interrupt handler doesn't take any locks.
> Or are spinlocks not allowed in softirq context either?
They are allowed there...
So, it has to be something different.
> > I believe I have to look at the latest set of patches, and try to find
> > any regressions. Do you have a location somewhere where I can download
> > the latest versions? Or do I need to dig through LKML to find the
> > latest... ;-)
> They are in -mm. You were Cc'ed I think...
Yes, I saw them, but I did not know if there were any updates in the
mean time, because I had seen some discussions, which confused me a
bit about the current status of the complete set.
> - with full preemption it runs but the serial line can't be used for
> receiving at high bit rate (using lrz)
A few questions arise here to me:
* What serial port is used here? (DBGU, or something else)
* No DMA was used, was flow-control enabled? (cannot with DBGU)
* If some other UART, why not using DMA?
Notice that the DBGU has no flow control, and just a 1 byte FIFO (thus
no fifo at all).
At high speeds (e.g. >=115200) it is _likely_ that you will miss
characters, nothing can prevent that. DBGU should only be used at
lower speeds, or just as text console. 115200 is running fine here as
text-console.
I would not expect that the behaviour is worse than without the
patchset, because without it it does not work at all on Preempt-RT,
but also: there was done much more in interrupt context previously, so
the chance of buffer overruns was much more likely in the old
situation.
The real interrupt handler (doing the reading from the fifo) must be
as short as possible, to be able to keep up with the data flow.
A simple calculation: 115200bps results in approx. 11520 bytes per second.
This means that the interrupt handler must be capable of handling each
byte on DBGU within 87us. With a worst case interrupt latency of about
85us, and average between 2us and 54us (on Preempt-RT and AT91RM9200),
you can simply understand that this will not match, how good/fast the
interrupt handling will ever be.
So, I suggest to either use flow-control, or DMA for bulkdata... (thus not DBGU)
Kind Regards,
Remy
On Wed, 30 Jan 2008 11:29:59 +0100
michael <[email protected]> wrote:
> > Now, _that_ is strange. I can't see anything that needs protection
> > across that call; in fact, I think we can lock a lot less than what we
> > currently do.
> >
> >
> I explain it bad:
> - with spin_lock the system seems, there is no problem with Valuntary
> Preeption and
> Preemptible Kernel
> - with full preemption it runs but the serial line can't be used for
> receiving at
> high bit rate (using lrz)
...but if you drop the spinlock across the call to
tty_flip_buffer_push, you get an Oops?
Could you post the Oops?
> >> Complete Preemption (Real-Time) ok but the serials is just unusable due
> >> to too many overruns (just using lrz)
> >>
> >
> > Is it worse than before? IIRC Remy mentioned something about
> > IRQF_NODELAY being the reason for moving all this code to softirq
> > context in the first place; does the interrupt handler run in hardirq
> > context?
> >
> >
> In the complete preemption yes.
Which question did you answer "yes" to? That it's worse than before or
that the interrupt handler runs in hardirq context (i.e. IRQF_NODELAY)?
> > I think you're right. Can you change it and see if it helps?
> >
> >
> I just change it because I have corruption on receiving buffer. All
> my test are done with this fix
Ok.
> > I guess I didn't test it thoroughly enough with DMA
> > disabled...slub_debug ought to catch such things, but not until we
> > receive enough data to actually overflow the buffer.
> >
> >
> I just test it I don't have
> buffer overflow.
No, I'd expect your allocation fix to take care of that. Or did you by
any chance test without the fix and with slub_debug enabled?
> I protect with a spinlock the access to the register when we sending
> from the tasklet. It is correct?
I have no idea. Could you post some more specifics about what you
modified, for example a diff?
Most of the tasklet is already protected by the spinlock, so you must
be careful to avoid any lock recursion.
Haavard
On Wed, 30 Jan 2008 12:05:40 +0100
"Remy Bohmer" <[email protected]> wrote:
> > > I believe I have to look at the latest set of patches, and try to find
> > > any regressions. Do you have a location somewhere where I can download
> > > the latest versions? Or do I need to dig through LKML to find the
> > > latest... ;-)
> > They are in -mm. You were Cc'ed I think...
>
> Yes, I saw them, but I did not know if there were any updates in the
> mean time, because I had seen some discussions, which confused me a
> bit about the current status of the complete set.
There's been one update, atmel_serial-add-dma-support-fix.patch, which
you were also Cc'ed on.
Haavard
Hi,
> A few questions arise here to me:
> * What serial port is used here? (DBGU, or something else)
> * No DMA was used, was flow-control enabled? (cannot with DBGU)
> * If some other UART, why not using DMA?
>
>
DBGU, so no flow control
> Notice that the DBGU has no flow control, and just a 1 byte FIFO (thus
> no fifo at all).
> At high speeds (e.g. >=115200) it is _likely_ that you will miss
> characters, nothing can prevent that. DBGU should only be used at
> lower speeds, or just as text console. 115200 is running fine here as
> text-console.
>
Overrun are admitted using DBGU and UART1..n without flow control,
but with the old version of the driver I can send a file using lrz
and with the new and full preemption is impossible.
> I would not expect that the behaviour is worse than without the
> patchset, because without it it does not work at all on Preempt-RT,
> but also: there was done much more in interrupt context previously, so
> the chance of buffer overruns was much more likely in the old
> situation.
> The real interrupt handler (doing the reading from the fifo) must be
> as short as possible, to be able to keep up with the data flow.
>
> A simple calculation: 115200bps results in approx. 11520 bytes per second.
> This means that the interrupt handler must be capable of handling each
> byte on DBGU within 87us. With a worst case interrupt latency of about
> 85us, and average between 2us and 54us (on Preempt-RT and AT91RM9200),
> you can simply understand that this will not match, how good/fast the
> interrupt handling will ever be.
>
> So, I suggest to either use flow-control, or DMA for bulkdata... (thus not DBGU)
>
> Kind Regards,
>
> Remy
>
>
Hi,
> On Wed, 30 Jan 2008 11:29:59 +0100
> michael <[email protected]> wrote:
>
>>> Now, _that_ is strange. I can't see anything that needs protection
>>> across that call; in fact, I think we can lock a lot less than what we
>>> currently do.
>>>
>>>
>>>
>> I explain it bad:
>> - with spin_lock the system seems, there is no problem with Valuntary
>> Preeption and
>> Preemptible Kernel
>> - with full preemption it runs but the serial line can't be used for
>> receiving at
>> high bit rate (using lrz)
>>
>
> ...but if you drop the spinlock across the call to
> tty_flip_buffer_push, you get an Oops?
>
> Could you post the Oops?
>
>
So this code
/*
* Drop the lock here since it might end up calling
* uart_start(), which takes the lock.
*/
spin_unlock(&port->lock);
tty_flip_buffer_push(port->info->tty);
spin_lock(&port->lock);
Works with:
CONFIG_PREEMPT_RT=y
CONFIG_PREEMPT=y
CONFIG_PREEMPT_SOFTIRQS=y
CONFIG_PREEMPT_HARDIRQS=y
CONFIG_PREEMPT_BKL=y
but crash with:
# CONFIG_PREEMPT_NONE is not set
CONFIG_PREEMPT_VOLUNTARY=y
# CONFIG_PREEMPT_DESKTOP is not set
# CONFIG_PREEMPT_RT is not set
CONFIG_PREEMPT_SOFTIRQS=y
# CONFIG_PREEMPT_HARDIRQS is not set
# CONFIG_PREEMPT_BKL is not set
CONFIG_CLASSIC_RCU=y
Seems to work in the last config if I comment the spin_lock &
spin_unlock call.
/*
* Drop the lock here since it might end up calling
* uart_start(), which takes the lock.
spin_unlock(&port->lock); */
tty_flip_buffer_push(port->info->tty);
/* spin_lock(&port->lock); */
It is not readable because I can't compile it with debugging information
(poor memory system)
>>>> Complete Preemption (Real-Time) ok but the serials is just unusable due
>>>> to too many overruns (just using lrz)
>>>>
>>>>
>>> Is it worse than before? IIRC Remy mentioned something about
>>> IRQF_NODELAY being the reason for moving all this code to softirq
>>> context in the first place; does the interrupt handler run in hardirq
>>> context?
>>>
>>>
>>>
>> In the complete preemption yes.
>>
>
> Which question did you answer "yes" to? That it's worse than before or
> that the interrupt handler runs in hardirq context (i.e. IRQF_NODELAY)?
>
>
The interrupt handler run in IRQF_NODELAY context.
>>> I think you're right. Can you change it and see if it helps?
>>>
>>>
>>>
>> I just change it because I have corruption on receiving buffer. All
>> my test are done with this fix
>>
>
> Ok.
>
>
>>> I guess I didn't test it thoroughly enough with DMA
>>> disabled...slub_debug ought to catch such things, but not until we
>>> receive enough data to actually overflow the buffer.
>>>
>>>
>>>
>> I just test it I don't have
>> buffer overflow.
>>
>
> No, I'd expect your allocation fix to take care of that. Or did you by
> any chance test without the fix and with slub_debug enabled?
>
>
I just meant that the buffer never fills up to its size.
>> I protect with a spinlock the access to the register when we sending
>> from the tasklet. It is correct?
>>
>
> I have no idea. Could you post some more specifics about what you
> modified, for example a diff?
>
>
...
/* The interrupt handler does not take the lock */
spin_lock_irqsave(&port->lock, flags);
atmel_tx_chars(port);
spin_unlock_irqrestore(&port->lock, flags);
spin_lock(&port->lock);
...
The atmel_tx_chars using the serial device registers like the interrupt
routine
and so I think that it is possible to have interference during send
operation.
> Most of the tasklet is already protected by the spinlock, so you must
> be careful to avoid any lock recursion.
>
> Haavard
>
>
Regards Michael
On Wed, 30 Jan 2008 16:26:27 +0100
michael <[email protected]> wrote:
> > I have no idea. Could you post some more specifics about what you
> > modified, for example a diff?
> >
> >
>
> ...
> /* The interrupt handler does not take the lock */
> spin_lock_irqsave(&port->lock, flags);
> atmel_tx_chars(port);
> spin_unlock_irqrestore(&port->lock, flags);
Sorry, this isn't going to work.
Please post a diff with the changes you did to the driver, and whatever
output you got when it crashed.
It's really difficult to help you when I don't know (a) what code
you're actually running, or (b) anything about the crash.
> The atmel_tx_chars using the serial device registers like the interrupt
> routine
> and so I think that it is possible to have interference during send
> operation.
No, it's only called from the tasklet, and the interrupt handler doesn't
touch the TX data register. There shouldn't be any need to disable
interrupts around the call to atmel_tx_chars(). In fact, this may very
well be the cause of the overruns you're seeing.
Haavard
Hi,
Haavard Skinnemoen wrote:
> On Wed, 30 Jan 2008 16:26:27 +0100
> michael <[email protected]> wrote:
>
>
>>> I have no idea. Could you post some more specifics about what you
>>> modified, for example a diff?
>>>
>>>
>>>
>> ...
>> /* The interrupt handler does not take the lock */
>> spin_lock_irqsave(&port->lock, flags);
>> atmel_tx_chars(port);
>> spin_unlock_irqrestore(&port->lock, flags);
>>
>
> Sorry, this isn't going to work.
>
> Please post a diff with the changes you did to the driver, and whatever
> output you got when it crashed.
>
> It's really difficult to help you when I don't know (a) what code
> you're actually running, or (b) anything about the crash.
>
>
Ok, but the problem is that I have some added code for using the uart with
smart card in iso mode, (is never called) and the patch is not so clean.
Now I return to the original patch without the spin_lock_irqsave and with
the fix of buffer allocation,and I don't see the crash anymore.
In full preemptive all works with threading hardirqs and sofirqs. I will
do other testing before posting again.
>> The atmel_tx_chars using the serial device registers like the interrupt
>> routine
>> and so I think that it is possible to have interference during send
>> operation.
>>
>
> No, it's only called from the tasklet, and the interrupt handler doesn't
> touch the TX data register. There shouldn't be any need to disable
> interrupts around the call to atmel_tx_chars(). In fact, this may very
> well be the cause of the overruns you're seeing.
>
> Haavard
>
>
The overrun still remain. An lrz receive session is impossible using
full preemption. I will try the dma patch too and test in iso mode for
smart card.
Thanks
On Thu, 31 Jan 2008 02:53:50 +0100
michael <[email protected]> wrote:
> The overrun still remain. An lrz receive session is impossible using
> full preemption. I will try the dma patch too and test in iso mode for
> smart card.
Hmm. Seems to work reasonably well on a non-rt kernel -- I get quite a
few overruns, but nothing more than the Zmodem protocol can handle.
It seems to be very sensitive to network traffic though...could it have
something to do with softirq scheduling? Could you try the patch below
and see if you can trigger the error message?
Haavard
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 477950f..c61fcc3 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -337,9 +337,12 @@ atmel_buffer_rx_char(struct uart_port *port, unsigned int status,
struct circ_buf *ring = &atmel_port->rx_ring;
struct atmel_uart_char *c;
- if (!CIRC_SPACE(ring->head, ring->tail, ATMEL_SERIAL_RINGSIZE))
+ if (!CIRC_SPACE(ring->head, ring->tail, ATMEL_SERIAL_RINGSIZE)) {
+ dev_err(port->dev, "RX ring buffer full, dropping data\n");
+
/* Buffer overflow, ignore char */
return;
+ }
c = &((struct atmel_uart_char *)ring->buf)[ring->head];
c->status = status;
Hello Haavard,
> It seems to be very sensitive to network traffic though...could it have
> something to do with softirq scheduling? Could you try the patch below
> and see if you can trigger the error message?
Funny that you mention this.
The largest latencies I currently have on RT (and rm9200) occur when
using a telnet session or NFS filesystems, thus while using network.
The impact on hardware Interrupt latencies are limited (<85us), so the
interrupt handler should still be able to keep up the receive buffer,
but context switches between threads can stall for a longer time under
some conditions.
A long shot, but can it be that the ringbuffer overflows, and that
therefor characters are lost?
Kind Regards,
Remy
On Thu, 31 Jan 2008 20:36:25 +0100
"Remy Bohmer" <[email protected]> wrote:
> A long shot, but can it be that the ringbuffer overflows, and that
> therefor characters are lost?
That's what I was thinking too. If this is indeed the cause, the
dev_err() added by the debug patch I posted should trigger and we may
consider boosting the priority of the tasklet (using
tasklet_hi_schedule.)
Other solutions may involve trying to figure out what exactly is
blocking the tasklet from running. I have a patch series for the macb
driver that optimizes the RX processing a bit, but it obviously won't
help at91rm9200 since it uses a different driver...
Haavard
Hi
> Haavard
>
> diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
> index 477950f..c61fcc3 100644
> --- a/drivers/serial/atmel_serial.c
> +++ b/drivers/serial/atmel_serial.c
> @@ -337,9 +337,12 @@ atmel_buffer_rx_char(struct uart_port *port, unsigned int status,
> struct circ_buf *ring = &atmel_port->rx_ring;
> struct atmel_uart_char *c;
>
> - if (!CIRC_SPACE(ring->head, ring->tail, ATMEL_SERIAL_RINGSIZE))
> + if (!CIRC_SPACE(ring->head, ring->tail, ATMEL_SERIAL_RINGSIZE)) {
> + dev_err(port->dev, "RX ring buffer full, dropping data\n");
> +
> /* Buffer overflow, ignore char */
> return;
> + }
>
> c = &((struct atmel_uart_char *)ring->buf)[ring->head];
> c->status = status;
>
>
I have already tried that but I have never seen the buffer full. So
tomorrow I can do other
tests with the serial device. I think the the atmel_interrupt handler
must check the
pass_counter before return IRQ_HANDLED.
Regards Michael
Hello Haavard,
> That's what I was thinking too. If this is indeed the cause, the
> dev_err() added by the debug patch I posted should trigger and we may
> consider boosting the priority of the tasklet (using
> tasklet_hi_schedule.)
Notice that we are talking about Preempt-RT here. Everything is
running in thread context, even tasklets, softirqs etc. They are _all_
preemptible, and if Michael has some RT-thread in the system that has
a higher priority than this tasklet or softirq, than the buffer will
eventually overflow.
I wonder also if Michael has set the RT-priorities correctly, on RT
_every_ softirq/irq thread starts by default on priority 50,
SCHED_FIFO. If they are still at 50, any other softirq/tasklet, or irq
can make this behavior worse.
Notice that the default 50 thingy rarely gives a decent behaving
system. (But any other default will also give problems anyway, so it
_has_ to be customized/tuned by the end user)
Kind Regards,
Remy
On Mon, 04 Feb 2008 20:01:26 +0100
michael <[email protected]> wrote:
> I think the the atmel_interrupt handler
> must check the
> pass_counter before return IRQ_HANDLED.
I'm not sure if it helps in this particular case but yeah, since the
interrupt may be shared, it's definitely wrong to always return
IRQ_HANDLED.
Nice catch. Could you try the patch below?
Haavard
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index cb70cc5..f310a80 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -552,7 +552,7 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id)
atmel_handle_transmit(port, pending);
} while (pass_counter++ < ATMEL_ISR_PASS_LIMIT);
- return IRQ_HANDLED;
+ return pass_counter ? IRQ_HANDLED : IRQ_NONE;
}
/*
hi,
> diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
> index cb70cc5..f310a80 100644
> --- a/drivers/serial/atmel_serial.c
> +++ b/drivers/serial/atmel_serial.c
> @@ -552,7 +552,7 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id)
> atmel_handle_transmit(port, pending);
> } while (pass_counter++ < ATMEL_ISR_PASS_LIMIT);
>
> - return IRQ_HANDLED;
> + return pass_counter ? IRQ_HANDLED : IRQ_NONE;
> }
>
> /*
>
>
Just one question:
Receiving with hardware handshake works without PDC?
Regards Michael
Hi,
the serial driver works fine. The problem seems to be related to the
tclib, when I use
it as a clocksource. The numbers of overruns depends on the type of
files too.
It is possible?
Regards
Michael
On Tue, 05 Feb 2008 13:29:35 +0100
michael <[email protected]> wrote:
> Just one question:
> Receiving with hardware handshake works without PDC?
I don't know...I haven't tried. These patches shouldn't change anything
though.
Haavard
Hi,
Haavard Skinnemoen wrote:
> On Tue, 05 Feb 2008 13:29:35 +0100
> michael <[email protected]> wrote:
>
>
>> Just one question:
>> Receiving with hardware handshake works without PDC?
>>
>
> I don't know...I haven't tried. These patches shouldn't change anything
> though.
>
> Haavard
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
I refer to this part of documentation:
"The USART behavior when hardware handshaking is enabled is the same as
the behavior in
standard synchronous or asynchronous mode, except that the receiver
drives the RTS pin as
described below and the level on the CTS pin modifies the behavior of
the transmitter as
described below. Using this mode requires using the PDC channel for
reception. The transmitter
can handle hardware handshaking in any case."
Regards Michael
On Wed, 06 Feb 2008 14:41:09 +0100
michael <[email protected]> wrote:
> I refer to this part of documentation:
>
> "The USART behavior when hardware handshaking is enabled is the same as
> the behavior in
> standard synchronous or asynchronous mode, except that the receiver
> drives the RTS pin as
> described below and the level on the CTS pin modifies the behavior of
> the transmitter as
> described below. Using this mode requires using the PDC channel for
> reception. The transmitter
> can handle hardware handshaking in any case."
Oh. I guess the answer is no, then.
Haavard
Hello Michael,
> the serial driver works fine. The problem seems to be related to the
> tclib, when I use it as a clocksource.
tclib as a clocksource should be no problem on Preempt-RT, do not use
it as clockevent device, it will not work properly on preempt-rt on
at91* yet, especially with the NO_HZ config.
> The numbers of overruns depends on the type of
> files too. It is possible?
I am still very curious to your (RT-) thread-prio layout. The problems
you mention can also be caused by a weird configuration of these
threads on preempt-rt.
Remy
2008/2/6, michael <[email protected]>:
> Hi,
>
> the serial driver works fine. The problem seems to be related to the
> tclib, when I use
> it as a clocksource. The numbers of overruns depends on the type of
> files too.
> It is possible?
>
> Regards
> Michael
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Hi,
All works now for me with preempt-rt. The problem is using hrtimer.
I think that hrtimer are executed with interrupts disabled so, if
this happen when I must receive a char, i have an overrun. The only
solution was the dma support to serial device.
Regards Michael
Hello All,
> All works now for me with preempt-rt. The problem is using hrtimer.
> I think that hrtimer are executed with interrupts disabled so, if
> this happen when I must receive a char, i have an overrun.
No, they share the same interrupt line...
So, while the timer interrupt handler is running, the serial handler
has to wait until the timer interrupt handler has finished.
Notice that the HRT interrupt handler is quite heavy and takes a long
time to complete.
And, as I already mentioned, related to the 1 byte FIFO and a
interrupt latency of about 85us (without HRT), it is logical that you
can get an overrun at the higher serial speeds... (>=115200bps)
> The only solution was the dma support to serial device.
Or, use flow control?
Kind Regards,
Remy
Hi,
Remy Bohmer wrote:
> Hello All,
>
>
>> All works now for me with preempt-rt. The problem is using hrtimer.
>> I think that hrtimer are executed with interrupts disabled so, if
>> this happen when I must receive a char, i have an overrun.
>>
>
> No, they share the same interrupt line...
>
I think that the hrtimer use and other interrupt line. The
AT91SAM9260_ID_TC2.
> So, while the timer interrupt handler is running, the serial handler
> has to wait until the timer interrupt handler has finished.
> Notice that the HRT interrupt handler is quite heavy and takes a long
> time to complete.
>
The problem is the heavy of HRT interrupt handler of course.
> And, as I already mentioned, related to the 1 byte FIFO and a
> interrupt latency of about 85us (without HRT), it is logical that you
> can get an overrun at the higher serial speeds... (>=115200bps)
>
>
I don't have the same problem without the hrtimer, I suppose the the
timer latency
is not so heavy.
>> The only solution was the dma support to serial device.
>>
>
> Or, use flow control?
>
>
>
Yes :)
Michael