lkml.org 
[lkml]   [2008]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -mm v4 7/9] atmel_serial: Add DMA support
> On Thu, 24 Jan 2008 13:41:49 +0100 Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> From: Chip Coldwell <coldwell@redhat.com>
>
> 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);
> +}
> +



\
 
 \ /
  Last update: 2008-01-27 07:07    [W:1.836 / U:0.984 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site