2007-12-18 17:08:38

by Haavard Skinnemoen

[permalink] [raw]
Subject: [PATCH 0/5] atmel_serial: Cleanups, irq handler splitup & DMA

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.

It all seems to behave both with and without DMA enabled, but I'll do
some more testing tomorrow.

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). I'm tempted to
just ignore the problem for now and hopefully come up with a solution
later.

Everyone, please give it a try and/or review the code.

PS: Andrew, I'm sending it to you because I believe you're the
maintainer of this driver, although MAINTAINERS doesn't say anything
about it. Please let me know if this isn't how you want it.

Chip Coldwell (1):
atmel_serial: Add DMA support

Haavard Skinnemoen (2):
atmel_serial: Use cpu_relax() when busy-waiting
atmel_serial: Use existing console options only if BRG is running

Remy Bohmer (2):
atmel_serial: Clean up the code
atmel_serial: Split the interrupt handler

drivers/serial/atmel_serial.c | 881 ++++++++++++++++++++++++++++++++---------
1 files changed, 700 insertions(+), 181 deletions(-)


2007-12-18 17:07:08

by Haavard Skinnemoen

[permalink] [raw]
Subject: [PATCH 4/5] atmel_serial: Split the interrupt handler

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.

The Transmit path was IRQF_NODELAY safe by itself, and is not adapted.
The read path for DMA(PDC) is also not adapted, because that code
does not run in IRQF_NODELAY context due to irq-sharing. The DBGU
which is shared with the timer-irq does not work with DMA, so
therefor this is no problem.

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 | 215 ++++++++++++++++++++++++++++++++---------
1 files changed, 169 insertions(+), 46 deletions(-)

diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index a6b3828..990d3ab 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -111,6 +111,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.
*/
@@ -119,6 +126,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_pending;
+ unsigned int irq_status;
+
+ struct circ_buf rx_ring;
};

static struct atmel_uart_port atmel_ports[ATMEL_MAX_UART];
@@ -248,22 +261,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
@@ -271,17 +304,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
@@ -294,33 +324,13 @@ 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);
}

/*
@@ -379,7 +389,7 @@ 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)
@@ -396,18 +406,16 @@ 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);
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
+
+ spin_lock(&port->lock);
+
+ atmel_port->irq_pending |= pending;
+ atmel_port->irq_status = status;
+
+ spin_unlock(&port->lock);
+
+ tasklet_schedule(&atmel_port->tasklet);
}

/*
@@ -434,6 +442,107 @@ 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);
+ }
+
+ tty_flip_buffer_push(port->info->tty);
+}
+
+/*
+ * 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 long flags;
+ unsigned int status;
+ unsigned int pending;
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ if (atmel_port->irq_pending) {
+ pending = atmel_port->irq_pending;
+ status = atmel_port->irq_status;
+ atmel_port->irq_pending = 0;
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ /* 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);
+ } else {
+ spin_unlock_irqrestore(&port->lock, flags);
+ }
+
+ atmel_rx_from_ring(port);
+}
+
/*
* Perform initialization and enable port for reception
*/
@@ -765,6 +874,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;
@@ -994,11 +1108,19 @@ 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);

+ data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+ port->rx_ring.buf = data;
+
ret = uart_add_one_port(&atmel_uart, &port->uart);
if (!ret) {
device_init_wakeup(&pdev->dev, 1);
@@ -1022,6 +1144,7 @@ static int __devexit atmel_serial_remove(struct platform_device *pdev)

if (port) {
ret = uart_remove_one_port(&atmel_uart, port);
+ kfree(atmel_port->rx_ring.buf);
kfree(port);
}

--
1.5.3.4

2007-12-18 17:07:34

by Haavard Skinnemoen

[permalink] [raw]
Subject: [PATCH 3/5] atmel_serial: Use existing console options only if BRG is running

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]>
---
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 61d71b0..a6b3828 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -842,13 +842,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)
@@ -868,7 +868,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.4

2007-12-18 17:08:19

by Haavard Skinnemoen

[permalink] [raw]
Subject: [PATCH 1/5] atmel_serial: Clean up the code

From: Remy Bohmer <[email protected]>

This patch cleans up the atmel_serial driver to conform the coding rules.
It contains no functional change.

[[email protected]: additional cleanups]
Signed-off-by: Remy Bohmer <[email protected]>
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
drivers/serial/atmel_serial.c | 327 ++++++++++++++++++++++++-----------------
1 files changed, 189 insertions(+), 138 deletions(-)

diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 111da57..bd41529 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -74,38 +74,42 @@

#define ATMEL_ISR_PASS_LIMIT 256

-#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)
-#define UART_PUT_IER(port,v) __raw_writel(v, (port)->membase + ATMEL_US_IER)
-#define UART_PUT_IDR(port,v) __raw_writel(v, (port)->membase + ATMEL_US_IDR)
-#define UART_GET_IMR(port) __raw_readl((port)->membase + ATMEL_US_IMR)
-#define UART_GET_CSR(port) __raw_readl((port)->membase + ATMEL_US_CSR)
-#define UART_GET_CHAR(port) __raw_readl((port)->membase + ATMEL_US_RHR)
-#define UART_PUT_CHAR(port,v) __raw_writel(v, (port)->membase + ATMEL_US_THR)
-#define UART_GET_BRGR(port) __raw_readl((port)->membase + ATMEL_US_BRGR)
-#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
+#define at_readl(port, off) __raw_readl((port)->membase + (off))
+#define at_writel(v, port, off) __raw_writel(v, (port)->membase + (off))
+
+#define UART_PUT_CR(port, v) at_writel(v, port, ATMEL_US_CR)
+#define UART_PUT_MR(port, v) at_writel(v, port, ATMEL_US_MR)
+#define UART_PUT_IER(port, v) at_writel(v, port, ATMEL_US_IER)
+#define UART_PUT_IDR(port, v) at_writel(v, port, ATMEL_US_IDR)
+#define UART_PUT_CHAR(port, v) at_writel(v, port, ATMEL_US_THR)
+#define UART_PUT_BRGR(port, v) at_writel(v, port, ATMEL_US_BRGR)
+#define UART_PUT_RTOR(port, v) at_writel(v, port, ATMEL_US_RTOR)
+
+#define UART_GET_MR(port) at_readl(port, ATMEL_US_MR)
+#define UART_GET_IMR(port) at_readl(port, ATMEL_US_IMR)
+#define UART_GET_CSR(port) at_readl(port, ATMEL_US_CSR)
+#define UART_GET_CHAR(port) at_readl(port, ATMEL_US_RHR)
+#define UART_GET_BRGR(port) at_readl(port, ATMEL_US_BRGR)
+
+/* is write-only */
+/* #define UART_GET_CR(port) at_readl(port, ATMEL_US_CR) */

/* 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)
-
-#define UART_PUT_RPR(port,v) __raw_writel(v, (port)->membase + ATMEL_PDC_RPR)
-#define UART_GET_RPR(port) __raw_readl((port)->membase + ATMEL_PDC_RPR)
-#define UART_PUT_RCR(port,v) __raw_writel(v, (port)->membase + ATMEL_PDC_RCR)
-#define UART_PUT_RNPR(port,v) __raw_writel(v, (port)->membase + ATMEL_PDC_RNPR)
-#define UART_PUT_RNCR(port,v) __raw_writel(v, (port)->membase + ATMEL_PDC_RNCR)
-
-#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 *);
+#define UART_PUT_PTCR(port, v) at_writel(v, port, ATMEL_PDC_PTCR)
+#define UART_PUT_RPR(port, v) at_writel(v, port, ATMEL_PDC_RPR)
+#define UART_PUT_RCR(port, v) at_writel(v, port, ATMEL_PDC_RCR)
+#define UART_PUT_RNPR(port, v) at_writel(v, port, ATMEL_PDC_RNPR)
+#define UART_PUT_RNCR(port, v) at_writel(v, port, ATMEL_PDC_RNCR)
+#define UART_PUT_TPR(port, v) at_writel(v, port, ATMEL_PDC_TPR)
+#define UART_PUT_TCR(port, v) at_writel(v, port, ATMEL_PDC_TCR)
+/*#define UART_PUT_TNPR(port, v) at_writel(v, port, ATMEL_PDC_TNPR) */
+/*#define UART_PUT_TNCR(port, v) at_writel(v, port, ATMEL_PDC_TNCR) */
+
+#define UART_GET_PTSR(port) at_readl(port, ATMEL_PDC_PTSR)
+#define UART_GET_RPR(port) at_readl(port, ATMEL_PDC_RPR)
+
+static int (*atmel_open_hook) (struct uart_port *);
+static void (*atmel_close_hook) (struct uart_port *);

/*
* We wrap our port structure around the generic uart_port.
@@ -142,8 +146,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 +232,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 +252,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 +271,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 +316,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 +357,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 +451,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 +474,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 +510,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 +578,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 +614,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 +749,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 +773,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 +798,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 +815,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 +880,11 @@ 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 == 0)
+ /* 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);

@@ -854,13 +899,13 @@ static int __init atmel_console_setup(struct console *co, char *options)
static struct uart_driver atmel_uart;

static struct console atmel_console = {
- .name = ATMEL_DEVICENAME,
- .write = atmel_console_write,
- .device = uart_console_device,
- .setup = atmel_console_setup,
- .flags = CON_PRINTBUFFER,
- .index = -1,
- .data = &atmel_uart,
+ .name = ATMEL_DEVICENAME,
+ .write = atmel_console_write,
+ .device = uart_console_device,
+ .setup = atmel_console_setup,
+ .flags = CON_PRINTBUFFER,
+ .index = -1,
+ .data = &atmel_uart,
};

#define ATMEL_CONSOLE_DEVICE &atmel_console
@@ -871,13 +916,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 +933,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 +947,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 +977,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 +1012,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.4

2007-12-18 17:08:57

by Haavard Skinnemoen

[permalink] [raw]
Subject: [PATCH 2/5] atmel_serial: Use cpu_relax() when busy-waiting

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]>
---
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 bd41529..61d71b0 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -623,7 +623,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);
@@ -802,7 +802,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.4

2007-12-18 17:09:22

by Haavard Skinnemoen

[permalink] [raw]
Subject: [PATCH 5/5] atmel_serial: Add DMA support

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

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/atmel_serial.c | 386 ++++++++++++++++++++++++++++++++++++++--
1 files changed, 366 insertions(+), 20 deletions(-)

diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 990d3ab..07c2734 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,11 @@

#include "atmel_serial.h"

+#define SUPPORT_PDC
+#define PDC_BUFFER_SIZE (L1_CACHE_BYTES << 3)
+#warning "Revisit"
+#define PDC_RX_TIMEOUT (3 * 10) /* 3 bytes */
+
#if defined(CONFIG_SERIAL_ATMEL_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
#define SUPPORT_SYSRQ
#endif
@@ -111,6 +119,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;
@@ -127,6 +142,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_pending;
unsigned int irq_status;
@@ -136,10 +158,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 SUPPORT_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.
*/
@@ -221,7 +272,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);
}

/*
@@ -229,7 +285,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);
}

/*
@@ -237,7 +303,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);
}

/*
@@ -286,6 +357,87 @@ 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++;
+}
+
+/*
+ * A transmission via the PDC is complete.
+ */
+static void atmel_pdc_endtx(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;
+
+ 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_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
+}
+
+/*
+ * The PDC transmitter is idle, so either start the next transfer or
+ * disable the transmitter.
+ */
+static void atmel_pdc_txbufe(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;
+
+ 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 */
+ UART_PUT_PTCR(port, ATMEL_PDC_TXTEN);
+ } else {
+ /* nothing left to transmit - disable the transmitter */
+
+ /* disable PDC transmit */
+ UART_PUT_PTCR(port, ATMEL_PDC_TXTDIS);
+ UART_PUT_IDR(port, ATMEL_US_ENDTX | ATMEL_US_TXBUFE);
+ }
+}
+
+/*
* Characters received (called from interrupt handler)
*/
static void atmel_rx_chars(struct uart_port *port)
@@ -374,6 +526,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);
@@ -394,6 +565,12 @@ atmel_handle_receive(struct uart_port *port, unsigned int pending)
static void
atmel_handle_transmit(struct uart_port *port, unsigned int pending)
{
+ /* PDC transmit */
+ if (pending & ATMEL_US_ENDTX)
+ atmel_pdc_endtx(port);
+ if (pending & ATMEL_US_TXBUFE)
+ atmel_pdc_txbufe(port);
+
/* Interrupt transmit */
if (pending & ATMEL_US_TXRDY)
atmel_tx_chars(port);
@@ -426,19 +603,17 @@ 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;
-
- status = UART_GET_CSR(port);
- pending = status & UART_GET_IMR(port);
- }
return IRQ_HANDLED;
}

@@ -504,6 +679,70 @@ static void atmel_rx_from_ring(struct uart_port *port)
tty_flip_buffer_push(port->info->tty);
}

+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);
+
+ tty_flip_buffer_push(tty);
+
+ UART_PUT_IER(port, ATMEL_US_ENDRX | ATMEL_US_TIMEOUT);
+}
+
/*
* tasklet handling tty stuff outside the interrupt handler.
*/
@@ -540,7 +779,10 @@ static void atmel_tasklet_func(unsigned long data)
spin_unlock_irqrestore(&port->lock, flags);
}

- atmel_rx_from_ring(port);
+ if (atmel_use_dma_rx(port))
+ atmel_rx_from_dma(port);
+ else
+ atmel_rx_from_ring(port);
}

/*
@@ -548,6 +790,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;

/*
@@ -568,6 +811,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)
*/
@@ -586,8 +879,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;
}
@@ -597,6 +900,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.
*/
@@ -708,6 +1043,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
*/
@@ -893,6 +1232,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;
}

/*
@@ -1077,7 +1421,7 @@ static int atmel_serial_suspend(struct platform_device *pdev,
struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;

if (device_may_wakeup(&pdev->dev)
- && !at91_suspend_entering_slow_clock())
+ && !clk_must_disable(atmel_port->clk))
enable_irq_wake(port->irq);
else {
uart_suspend_port(&atmel_uart, port);
@@ -1116,10 +1460,12 @@ static int __devinit atmel_serial_probe(struct platform_device *pdev)
port = &atmel_ports[pdev->id];
atmel_init_port(port, pdev);

- data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
- if (!data)
- return -ENOMEM;
- port->rx_ring.buf = data;
+ if (!atmel_use_dma_rx(&port->uart)) {
+ data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+ port->rx_ring.buf = data;
+ }

ret = uart_add_one_port(&atmel_uart, &port->uart);
if (!ret) {
--
1.5.3.4

2007-12-18 17:13:00

by Haavard Skinnemoen

[permalink] [raw]
Subject: Re: [PATCH 4/5] atmel_serial: Split the interrupt handler

On Tue, 18 Dec 2007 18:06:14 +0100
Haavard Skinnemoen <[email protected]> wrote:

> From: Remy Bohmer <[email protected]>

Heh. That's obviously wrong. Wonder what happened there?

Looks like Chip's address got mangled too.

Haavard

2007-12-18 18:22:01

by Chip Coldwell

[permalink] [raw]
Subject: Re: [PATCH 5/5] atmel_serial: Add DMA support

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue, 18 Dec 2007, Haavard Skinnemoen 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 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.
>
> 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/atmel_serial.c | 386 ++++++++++++++++++++++++++++++++++++++--
> 1 files changed, 366 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
> index 990d3ab..07c2734 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.

I will ACK/Sign-off on this soon; I just want to do some tests on real
hardware first.

Chip

- --
Charles M. Coldwell
"Turn on, log in, tune out"
Somerville, Massachusetts, New England
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)

iD8DBQFHaA7cr6maj4UuBS8RAjMSAJsGcKoFKCP/R3aAPhW5hj+v3Qt6ZACgshsF
5NP6/9+NbhDAxBC/7jo8J0Y=
=hx4t
-----END PGP SIGNATURE-----

2007-12-18 18:22:52

by Chip Coldwell

[permalink] [raw]
Subject: Re: [PATCH 4/5] atmel_serial: Split the interrupt handler

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue, 18 Dec 2007, Haavard Skinnemoen wrote:

> On Tue, 18 Dec 2007 18:06:14 +0100
> Haavard Skinnemoen <[email protected]> wrote:
>
> > From: Remy Bohmer <[email protected]>
>
> Heh. That's obviously wrong. Wonder what happened there?
>
> Looks like Chip's address got mangled too.

You can find me at <[email protected]> or <[email protected]> these
days, although <[email protected]> still works for the time
being.

Chip

- --
Charles M. "Chip" Coldwell
Senior Software Engineer
Red Hat, Inc
978-392-2426

GPG ID: 852E052F
GPG FPR: 77E5 2B51 4907 F08A 7E92 DE80 AFA9 9A8F 852E 052F
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)

iD8DBQFHaA87r6maj4UuBS8RApaiAKCDFvC/WtA/s0pysvMIZIsTlAcN7wCffRoa
JwA3E6Kt16pU9yi1dx1lCWk=
=M528
-----END PGP SIGNATURE-----

2007-12-18 23:15:43

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH 4/5] atmel_serial: Split the interrupt handler

On 12/18/2007 06:06 PM, Haavard Skinnemoen wrote:
> From: Remy Bohmer <[email protected]>
>
> This patch splits up the interrupt handler of the serial port
> into a interrupt top-half and a tasklet.
[...]
> [[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 | 215 ++++++++++++++++++++++++++++++++---------
> 1 files changed, 169 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
> index a6b3828..990d3ab 100644
> --- a/drivers/serial/atmel_serial.c
> +++ b/drivers/serial/atmel_serial.c
[...]
> @@ -994,11 +1108,19 @@ 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);
>
> + data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
> + if (!data)

{
clk_disable(atmel_port->clk);
clk_put(atmel_port->clk);


> + return -ENOMEM;

}

> + port->rx_ring.buf = data;
> +
> ret = uart_add_one_port(&atmel_uart, &port->uart);
> if (!ret) {
> device_init_wakeup(&pdev->dev, 1);
> @@ -1022,6 +1144,7 @@ static int __devexit atmel_serial_remove(struct platform_device *pdev)
>
> if (port) {
> ret = uart_remove_one_port(&atmel_uart, port);

Don't you need tasklet_kill() here (or somewhere)?

> + kfree(atmel_port->rx_ring.buf);
> kfree(port);
> }
>

2007-12-19 07:46:02

by Vadim Yatsenko

[permalink] [raw]
Subject: Re: [PATCH 0/5] atmel_serial: Cleanups, irq handler splitup & DMA

What prepatches one need to aply to what kernel version (vanila, git, ...)
before these?

With best regards, Vadim Yatsenko.

----- Original Message -----
From: "Haavard Skinnemoen" <[email protected]>
To: "Andrew Victor" <[email protected]>
Cc: "Russell King - ARM Linux" <[email protected]>; "Haavard
Skinnemoen" <[email protected]>; <[email protected]>;
<[email protected]>; "Remy Bohmer" <[email protected]>; "ARM Linux
Mailing List" <[email protected]>
Sent: Tuesday, December 18, 2007 7:06 PM
Subject: [PATCH 0/5] atmel_serial: Cleanups, irq handler splitup & DMA


> 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.
>
> It all seems to behave both with and without DMA enabled, but I'll do
> some more testing tomorrow.
>
> 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). I'm tempted to
> just ignore the problem for now and hopefully come up with a solution
> later.
>
> Everyone, please give it a try and/or review the code.
>
> PS: Andrew, I'm sending it to you because I believe you're the
> maintainer of this driver, although MAINTAINERS doesn't say anything
> about it. Please let me know if this isn't how you want it.
>
> Chip Coldwell (1):
> atmel_serial: Add DMA support
>
> Haavard Skinnemoen (2):
> atmel_serial: Use cpu_relax() when busy-waiting
> atmel_serial: Use existing console options only if BRG is running
>
> Remy Bohmer (2):
> atmel_serial: Clean up the code
> atmel_serial: Split the interrupt handler
>
> drivers/serial/atmel_serial.c | 881
> ++++++++++++++++++++++++++++++++---------
> 1 files changed, 700 insertions(+), 181 deletions(-)
>
> -------------------------------------------------------------------
> List admin:
> http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
> FAQ: http://www.arm.linux.org.uk/mailinglists/faq.php
> Etiquette: http://www.arm.linux.org.uk/mailinglists/etiquette.php
>

2007-12-19 09:50:51

by Haavard Skinnemoen

[permalink] [raw]
Subject: Re: [PATCH 4/5] atmel_serial: Split the interrupt handler

On Wed, 19 Dec 2007 00:15:24 +0100
Jiri Slaby <[email protected]> wrote:

> On 12/18/2007 06:06 PM, Haavard Skinnemoen wrote:
> > port = &atmel_ports[pdev->id];
> > atmel_init_port(port, pdev);
> >
> > + data = kmalloc(ATMEL_SERIAL_RINGSIZE, GFP_KERNEL);
> > + if (!data)
>
> {
> clk_disable(atmel_port->clk);
> clk_put(atmel_port->clk);

Indeed, thanks.

Hmm...the existing error path gets this wrong too. It's actually a
bit risky to disable the clock here since we might end up losing the
console.

I wonder what we're really supposed to do if the console is initialized
successfully, but the corresponding device fails to probe...?

> > @@ -1022,6 +1144,7 @@ static int __devexit atmel_serial_remove(struct platform_device *pdev)
> >
> > if (port) {
> > ret = uart_remove_one_port(&atmel_uart, port);
>
> Don't you need tasklet_kill() here (or somewhere)?

Absolutely. Thanks again.

> > + kfree(atmel_port->rx_ring.buf);
> > kfree(port);

Hmm...this actually looks like a bug too. "port" is allocated
statically, so it shouldn't be freed. I wonder if anyone ever use this
driver as a module?

Haavard

2007-12-19 09:54:38

by Haavard Skinnemoen

[permalink] [raw]
Subject: Re: [PATCH 0/5] atmel_serial: Cleanups, irq handler splitup & DMA

On Wed, 19 Dec 2007 09:29:52 +0200
"Vadim Yatsenko" <[email protected]> wrote:

> What prepatches one need to aply to what kernel version (vanila, git, ...)
> before these?

They should apply to a fresh checkout of Linus' git tree.

If you want to use 2.6.23, you need to apply this patch first:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff_plain;h=19cd7537bdae6685c31677a01e08850612ba87f6

Haavard

2007-12-19 11:41:17

by Haavard Skinnemoen

[permalink] [raw]
Subject: Re: [PATCH 4/5] atmel_serial: Split the interrupt handler

On Tue, 18 Dec 2007 13:19:39 -0500 (EST)
Chip Coldwell <[email protected]> wrote:

> > On Tue, 18 Dec 2007 18:06:14 +0100
> > Haavard Skinnemoen <[email protected]> wrote:
> >
> > > From: Remy Bohmer <[email protected]>
> >
> > Heh. That's obviously wrong. Wonder what happened there?
> >
> > Looks like Chip's address got mangled too.
>
> You can find me at <[email protected]> or <[email protected]> these
> days, although <[email protected]> still works for the time
> being.

Thanks. I'll update the broken commits.

Btw, the funny thing is that, looking at my shell history, I think I
actually did the right thing when committing your patches:

git commit -s --author 'Remy Bohmer <[email protected]>'
git commit -s --author 'Chip Coldwell <[email protected]>'

But for some reason only your names stuck, not your e-mail addresses...

Haavard

2007-12-19 14:07:47

by Haavard Skinnemoen

[permalink] [raw]
Subject: Re: [PATCH 4/5] atmel_serial: Split the interrupt handler

On Wed, 19 Dec 2007 12:40:08 +0100
Haavard Skinnemoen <[email protected]> wrote:

> Btw, the funny thing is that, looking at my shell history, I think I
> actually did the right thing when committing your patches:
>
> git commit -s --author 'Remy Bohmer <[email protected]>'
> git commit -s --author 'Chip Coldwell <[email protected]>'
>
> But for some reason only your names stuck, not your e-mail addresses...

It just happened again. Seems like if git-rebase hits a conflict, and I
just do "git-rebase --continue" after resolving it (and updating the
index), the original author's name is used, but not his e-mail address.

Looks like a bug in git-rebase...?

Haavard