Re: [PATCH -mm v4 7/9] atmel_serial: Add DMA support

[Andrew Morton <akpm@linux-foundation.org>]

> 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);
> +}
> +