From: David Brownell Subject: Fwd: Re: Fwd: [PATCH] atmel_spi: Allow chained transfers Date: Wed, 14 Nov 2007 10:57:17 -0800 Message-ID: <200711141057.17570.david-b@pacbell.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Return-path: Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: spi-devel-general-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: spi-devel-general-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: linux-spi.vger.kernel.org If this checks out for me too, I'll likely forward it for inclusion in the MM tree, and with a note to the ARM list. Silvester, thanks for this patch ... it sounds like it'll be a winner! ---------- Forwarded Message ---------- Subject: Re: Fwd: [spi-devel-general] [PATCH] atmel_spi: Allow chained transfers Date: Wednesday 14 November 2007 From: "Silvester Erdeg" To: "Haavard Skinnemoen" On Nov 13, 2007 3:04 PM, Haavard Skinnemoen wrote: > ... > The patch looks good, and it works fine on my NGW board with > mtd_dataflash and jffs2. I'm not sure if it's any faster, but if > someone with an AT91RM9200 board could verify that it solves the chip > select issues, I think it should be merged after some extensive testing. We tested this on an AT91RM9200 board with a fingerprint reader chip connected to spi.0. It solved our problems with it. But I agree that it should go through some more testing. > I can't find anything wrong or even suspicious about the patch, but it > looks like you're dereferencing as->current_transfer and > as->current_remaining_bytes quite a lot. I wonder if gcc is smart > enough to keep it in a register most of the time...? Also, > atmel_spi_next_xfer_data() takes a "len" pointer which is dereferenced > many times as well. That might hurt too, unless gcc is smart enough to > keep it in a register (which it should since the function looks like it > will be inlined anyway.) I checked the assembly source and gcc is not smart enough, so I reworked the patch to address these concerns. The source is perhaps a bit more difficult to read but it is more compact now. --------- This patch adds support for chained transfers in the atmel_spi driver. Almost all of the logic can be found in the reworked atmel_spi_next_xfer() function. When it is called, the driver is in one of the following three states: 1. It isn't transferring anything (in which case the first transfer of the current message is going to be sent) 2. It has finished transfering a non-chainable transfer (in which case it will go to the next transfer in the message) 3. It has finished transfering a chained transfer (in which case the next transfer is already queued) After that it will queue the next transfer if it can be chained. The patch is made against 2.6.24-rc2. Signed-off-by: Szilveszter Ordog diff -uNrp linux-2.6.24-rc1-orig/drivers/spi/atmel_spi.c linux-2.6.24-rc1/drivers/spi/atmel_spi.c --- linux-2.6.24-rc1-orig/drivers/spi/atmel_spi.c 2007-11-14 17:25:41.000000000 +0100 +++ linux-2.6.24-rc1/drivers/spi/atmel_spi.c 2007-11-14 17:31:25.000000000 +0100 @@ -51,7 +51,9 @@ struct atmel_spi { u8 stopping; struct list_head queue; struct spi_transfer *current_transfer; - unsigned long remaining_bytes; + unsigned long current_remaining_bytes; + struct spi_transfer *next_transfer; + unsigned long next_remaining_bytes; void *buffer; dma_addr_t buffer_dma; @@ -121,6 +123,48 @@ static void cs_deactivate(struct atmel_s gpio_set_value(gpio, !active); } +static inline int atmel_spi_xfer_is_last(struct spi_message *msg, + struct spi_transfer *xfer) +{ + return msg->transfers.prev == &xfer->transfer_list; +} + +static inline int atmel_spi_xfer_can_be_chained(struct spi_transfer *xfer) +{ + return xfer->delay_usecs == 0 && !xfer->cs_change; +} + +static void atmel_spi_next_xfer_data(struct spi_master *master, + struct spi_transfer *xfer, + dma_addr_t *tx_dma, + dma_addr_t *rx_dma, + u32 *plen) +{ + struct atmel_spi *as = spi_master_get_devdata(master); + u32 len = *plen; + + /* use scratch buffer only when rx or tx data is unspecified */ + if (xfer->rx_buf) + *rx_dma = xfer->rx_dma + xfer->len - len; + else { + *rx_dma = as->buffer_dma; + if (len > BUFFER_SIZE) + len = BUFFER_SIZE; + } + if (xfer->tx_buf) + *tx_dma = xfer->tx_dma + xfer->len - len; + else { + *tx_dma = as->buffer_dma; + if (len > BUFFER_SIZE) + len = BUFFER_SIZE; + memset(as->buffer, 0, len); + dma_sync_single_for_device(&as->pdev->dev, + as->buffer_dma, len, DMA_TO_DEVICE); + } + + *plen = len; +} + /* * Submit next transfer for DMA. * lock is held, spi irq is blocked @@ -130,53 +174,68 @@ static void atmel_spi_next_xfer(struct s { struct atmel_spi *as = spi_master_get_devdata(master); struct spi_transfer *xfer; - u32 len; + u32 len, remaining, total; dma_addr_t tx_dma, rx_dma; - xfer = as->current_transfer; - if (!xfer || as->remaining_bytes == 0) { - if (xfer) - xfer = list_entry(xfer->transfer_list.next, - struct spi_transfer, transfer_list); - else - xfer = list_entry(msg->transfers.next, - struct spi_transfer, transfer_list); - as->remaining_bytes = xfer->len; - as->current_transfer = xfer; + if (!as->current_transfer) + xfer = list_entry(msg->transfers.next, + struct spi_transfer, transfer_list); + else if (!as->next_transfer) + xfer = list_entry(as->current_transfer->transfer_list.next, + struct spi_transfer, transfer_list); + else + xfer = NULL; + + if (xfer) { + len = xfer->len; + atmel_spi_next_xfer_data(master, xfer, &tx_dma, &rx_dma, &len); + remaining = xfer->len - len; + + spi_writel(as, TPR, tx_dma); + spi_writel(as, RPR, rx_dma); + + if (msg->spi->bits_per_word > 8) + len >>= 1; + spi_writel(as, TCR, len); + spi_writel(as, RCR, len); + } else { + xfer = as->next_transfer; + remaining = as->next_remaining_bytes; } - len = as->remaining_bytes; + as->current_transfer = xfer; + as->current_remaining_bytes = remaining; - tx_dma = xfer->tx_dma + xfer->len - len; - rx_dma = xfer->rx_dma + xfer->len - len; + if (remaining > 0) + len = remaining; + else if (!atmel_spi_xfer_is_last(msg, xfer) && + atmel_spi_xfer_can_be_chained(xfer)) { + xfer = list_entry(xfer->transfer_list.next, + struct spi_transfer, transfer_list); + len = xfer->len; + } else + xfer = NULL; - /* use scratch buffer only when rx or tx data is unspecified */ - if (!xfer->rx_buf) { - rx_dma = as->buffer_dma; - if (len > BUFFER_SIZE) - len = BUFFER_SIZE; - } - if (!xfer->tx_buf) { - tx_dma = as->buffer_dma; - if (len > BUFFER_SIZE) - len = BUFFER_SIZE; - memset(as->buffer, 0, len); - dma_sync_single_for_device(&as->pdev->dev, - as->buffer_dma, len, DMA_TO_DEVICE); - } + as->next_transfer = xfer; - spi_writel(as, RPR, rx_dma); - spi_writel(as, TPR, tx_dma); + if (xfer) { + total = len; + atmel_spi_next_xfer_data(master, xfer, &tx_dma, &rx_dma, &len); + as->next_remaining_bytes = total - len; + + spi_writel(as, TNPR, tx_dma); + spi_writel(as, RNPR, rx_dma); + + if (msg->spi->bits_per_word > 8) + len >>= 1; + spi_writel(as, TNCR, len); + spi_writel(as, RNCR, len); + } else { + spi_writel(as, TNCR, 0); + spi_writel(as, RNCR, 0); + } - as->remaining_bytes -= len; - if (msg->spi->bits_per_word > 8) - len >>= 1; - - /* REVISIT: when xfer->delay_usecs == 0, the PDC "next transfer" - * mechanism might help avoid the IRQ latency between transfers - * (and improve the nCS0 errata handling on at91rm9200 chips) - * - * We're also waiting for ENDRX before we start the next + /* REVISIT: We're waiting for ENDRX before we start the next * transfer because we need to handle some difficult timing * issues otherwise. If we wait for ENDTX in one transfer and * then starts waiting for ENDRX in the next, it's difficult @@ -186,8 +245,6 @@ static void atmel_spi_next_xfer(struct s * * It should be doable, though. Just not now... */ - spi_writel(as, TNCR, 0); - spi_writel(as, RNCR, 0); spi_writel(as, IER, SPI_BIT(ENDRX) | SPI_BIT(OVRES)); dev_dbg(&msg->spi->dev, @@ -195,8 +252,6 @@ static void atmel_spi_next_xfer(struct s xfer, xfer->len, xfer->tx_buf, xfer->tx_dma, xfer->rx_buf, xfer->rx_dma, spi_readl(as, IMR)); - spi_writel(as, TCR, len); - spi_writel(as, RCR, len); spi_writel(as, PTCR, SPI_BIT(TXTEN) | SPI_BIT(RXTEN)); } @@ -294,6 +349,7 @@ atmel_spi_msg_done(struct spi_master *ma spin_lock(&as->lock); as->current_transfer = NULL; + as->next_transfer = NULL; /* continue if needed */ if (list_empty(&as->queue) || as->stopping) @@ -377,7 +433,7 @@ atmel_spi_interrupt(int irq, void *dev_i spi_writel(as, IDR, pending); - if (as->remaining_bytes == 0) { + if (as->current_remaining_bytes == 0) { msg->actual_length += xfer->len; if (!msg->is_dma_mapped) @@ -387,7 +443,7 @@ atmel_spi_interrupt(int irq, void *dev_i if (xfer->delay_usecs) udelay(xfer->delay_usecs); - if (msg->transfers.prev == &xfer->transfer_list) { + if (atmel_spi_xfer_is_last(msg, xfer)) { /* report completed message */ atmel_spi_msg_done(master, as, msg, 0, xfer->cs_change); ------------------------------------------------------- ------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/