spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

David:

I think enough time has elapsed for any objections to surface. Is there still time to push this patch for 2.6.27? The actual patch is contained in my email of 8/19/08:

[spi-devel-general] [Patch 1/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

Also, the patch should be directed at the stable maintainers for addition to any kernel as far back as 2.6.20. I'm not sure how to do direct that, but I expect that you do.

Thanks.

Ned Forrester                                       nforrester-/[email protected]
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

I enclose a slightly cleaned-up version. Comments, checkpath.pl, and so on. Could you eyeball make sure it still behaves?

Also, this is too much for one patch. The transfer_delay and cs_change stuff (fixing bugs #1-4) are plausibly one patch. And the DMA stuff (#5-6) is plausibly a different patch.

Could you split those two out? Then I'll submit them.

There should still be time; thanks for the ping.

One just cc's [email protected] ... I don't think the stable team maintains anything that old though. I cc'd Eric (at Marvell)
who is probably worth keeping in the loop in terms of PXA fixes.
The PXA platform is now getting an overhaul to help make the
PXA3xx changes work better ... yay!

- Dave

p.s. Eric, it seems the pxa2xx_spi driver is in need of more
active maintaining than it's got. I seem to have a few other patches for it in my mailbox too...

========= CUT HERE
From: Ned Forrester <nforrester-/[email protected]>

Fixes several related bugs in the pxa2xx_spi driver. These bugs are in all versions of this driver (except for #6, which was introduced in the 2.6.20 kernel) and prevent using it with chips like m25p16 flash.

1. The spi_transfer.cs_change flag is handled too early:
before spi_transfer.delay_usecs applies, thus making the delay ineffective at holding chip select.

2. spi_transfer.delay_usecs is ignored on the last transfer of a message (likewise not holding chipselect long enough).

3. If spi_transfer.cs_change is set on the last transfer, the chip select is always disabled, instead of the intended
meaning: optionally holding chip select enabled for the next message. (Impact at least MMC-over-SPI.)

Those first three bugs were fixed with a relocation of delays and chip select de-assertions.

4. If a message has the cs_change flag set on the last transfer, and had the chip select stayed enabled as requested (see 3, above), it would not have been disabled if the next message is for a different chip. Fixed by dropping chip select regardless of cs_change at end of a message, if there is no next message or if the next message is for a different chip.

5. Zero length transfers are permitted for use to insert timing, but pxa2xx_spi.c will fail if this is requested in DMA mode. Fixed by using programmed I/O (PIO) mode for such transfers.

6. Transfers larger than 8191 are not permitted in DMA mode. A test for length rejects all large transfers regardless of DMA or PIO mode. Worked around by rejecting only large transfers with DMA mapped buffers, and forcing all other transfers larger than 8191 to use PIO mode. A rate limited warning is issued for DMA transfers forced to PIO mode.

This patch should apply to all kernels back to and including 2.6.20; it was test patched against 2.6.20. An additional patch would be required for older kernels, but those versions are very buggy anyway.

There are some additional issues when GPIOs aren't being used as chipselects. Probably the SSFRM support should be removed, and the GPIO support switched over to use standard GPIO calls (and to support SPI_CS_HIGH).

# NYET-Signed-off-by: David Brownell <[email protected]>

---
 drivers/spi/pxa2xx_spi.c |  116 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 91 insertions(+), 25 deletions(-)

@@ -47,9 +47,10 @@ MODULE_ALIAS("platform:pxa2xx-spi");
 
 #define MAX_BUSES 3
 
-#define DMA_INT_MASK (DCSR_ENDINTR | DCSR_STARTINTR | DCSR_BUSERR)
-#define RESET_DMA_CHANNEL (DCSR_NODESC | DMA_INT_MASK)
-#define IS_DMA_ALIGNED(x) (((u32)(x)&0x07)==0)
+#define DMA_INT_MASK		(DCSR_ENDINTR | DCSR_STARTINTR | DCSR_BUSERR)
+#define RESET_DMA_CHANNEL	(DCSR_NODESC | DMA_INT_MASK)
+#define IS_DMA_ALIGNED(x)	(((x) & 0x07) == 0)
+#define MAX_DMA_LEN		8191
 
 /*
  * for testing SSCR1 changes that require SSP restart, basically

@@ -144,7 +145,6 @@ struct driver_data {
 	size_t tx_map_len;
 	u8 n_bytes;
 	u32 dma_width;
-	int cs_change;
 	int (*write)(struct driver_data *drv_data);
 	int (*read)(struct driver_data *drv_data);
 	irqreturn_t (*transfer_handler)(struct driver_data *drv_data);

@@ -406,8 +406,45 @@ static void giveback(struct driver_data 
 					struct spi_transfer,
 					transfer_list);
 
+	/* Delay if requested before any change in chip select */
+	if (last_transfer->delay_usecs)
+		udelay(last_transfer->delay_usecs);
+
+	/* Drop chip select UNLESS cs_change is true or we are returning
+	 * a message with an error, or next message is for another chip
+	 */
 	if (!last_transfer->cs_change)
 		drv_data->cs_control(PXA2XX_CS_DEASSERT);
+	else {
+		struct spi_message *next_msg;
+
+		/* Holding of cs was hinted, but we need to make sure
+		 * the next message is for the same chip.  Don't waste
+		 * time with the following tests unless this was hinted.
+		 *
+		 * We cannot postpone this until pump_messages, because
+		 * after calling msg->complete (below) the driver that
+		 * sent the current message could be unloaded, which
+		 * could invalidate the cs_control() callback...
+		 */
+
+		/* get a pointer to the next message, if any */
+		spin_lock_irqsave(&drv_data->lock, flags);
+		if (list_empty(&drv_data->queue))
+			next_msg = NULL;
+		else
+			next_msg = list_entry(drv_data->queue.next,
+					struct spi_message, queue);
+		spin_unlock_irqrestore(&drv_data->lock, flags);
+
+		/* see if the next and current messages point
+		 * to the same chip
+		 */
+		if (next_msg && next_msg->spi != msg->spi)
+			next_msg = NULL;
+		if (!next_msg || msg->state == ERROR_STATE)
+			drv_data->cs_control(PXA2XX_CS_DEASSERT);
+	}
 
 	msg->state = NULL;
 	if (msg->complete)

@@ -490,10 +527,9 @@ static void dma_transfer_complete(struct
 	msg->actual_length += drv_data->len -
 				(drv_data->rx_end - drv_data->rx);
 
-	/* Release chip select if requested, transfer delays are
-	 * handled in pump_transfers */
-	if (drv_data->cs_change)
-		drv_data->cs_control(PXA2XX_CS_DEASSERT);
+	/* Transfer delays and chip select release are
+	 * handled in pump_transfers or giveback
+	 */
 
 	/* Move to next transfer */
 	msg->state = next_transfer(drv_data);

@@ -602,10 +638,9 @@ static void int_transfer_complete(struct
 	drv_data->cur_msg->actual_length += drv_data->len -
 				(drv_data->rx_end - drv_data->rx);
 
-	/* Release chip select if requested, transfer delays are
-	 * handled in pump_transfers */
-	if (drv_data->cs_change)
-		drv_data->cs_control(PXA2XX_CS_DEASSERT);
+	/* Transfer delays and chip select release are
+	 * handled in pump_transfers or giveback
+	 */
 
 	/* Move to next transfer */
 	drv_data->cur_msg->state = next_transfer(drv_data);

@@ -840,23 +875,40 @@ static void pump_transfers(unsigned long
 		return;
 	}
 
-	/* Delay if requested at end of transfer*/
+	/* Delay if requested at end of transfer before CS change */
 	if (message->state == RUNNING_STATE) {
 		previous = list_entry(transfer->transfer_list.prev,
 					struct spi_transfer,
 					transfer_list);
 		if (previous->delay_usecs)
 			udelay(previous->delay_usecs);
+
+		/* Drop chip select only if cs_change is requested */
+		if (previous->cs_change)
+			drv_data->cs_control(PXA2XX_CS_DEASSERT);
 	}
 
-	/* Check transfer length */
-	if (transfer->len > 8191)
-	{
-		dev_warn(&drv_data->pdev->dev, "pump_transfers: transfer "
-				"length greater than 8191\n");
-		message->status = -EINVAL;
-		giveback(drv_data);
-		return;
+	/* Check for transfers that need multiple DMA segments */
+	if (transfer->len > MAX_DMA_LEN && chip->enable_dma) {
+
+		/* reject already-mapped transfers; PIO won't always work */
+		if (message->is_dma_mapped
+				|| transfer->rx_dma || transfer->tx_dma) {
+			dev_err(&drv_data->pdev->dev,
+				"pump_transfers: mapped transfer length "
+				"of %lu is greater than %d\n",
+				transfer->len, MAX_DMA_LEN);
+			message->status = -EINVAL;
+			giveback(drv_data);
+			return;
+		}
+
+		/* warn ... we force this to PIO mode */
+		if (printk_ratelimit())
+			dev_warn(&message->spi->dev, "pump_transfers: "
+				"DMA disabled for transfer length %ld "
+				"greater than %d\n",
+				(long)drv_data->len, MAX_DMA_LEN);
 	}
 
 	/* Setup the transfer state based on the type of transfer */

@@ -878,7 +930,6 @@ static void pump_transfers(unsigned long
 	drv_data->len = transfer->len & DCMD_LENGTH;
 	drv_data->write = drv_data->tx ? chip->write : null_writer;
 	drv_data->read = drv_data->rx ? chip->read : null_reader;
-	drv_data->cs_change = transfer->cs_change;
 
 	/* Change speed and bit per word on a per transfer */
 	cr0 = chip->cr0;

@@ -925,7 +976,7 @@ static void pump_transfers(unsigned long
 							&dma_thresh))
 				if (printk_ratelimit())
 					dev_warn(&message->spi->dev,
-						"pump_transfer: "
+						"pump_transfers: "
 						"DMA burst size reduced to "
 						"match bits_per_word\n");
 		}

@@ -939,8 +990,23 @@ static void pump_transfers(unsigned long
 
 	message->state = RUNNING_STATE;
 
-	/* Try to map dma buffer and do a dma transfer if successful */
-	if ((drv_data->dma_mapped = map_dma_buffers(drv_data))) {
+	/* Try to map dma buffer and do a dma transfer if successful, but
+	 * only if the length is non-zero and less than MAX_DMA_LEN.
+	 *
+	 * Zero-length non-descriptor DMA is illegal on PXA2xx; force use
+	 * of PIO instead.  Care is needed above because the transfer may
+	 * have have been passed with buffers that are already dma mapped.
+	 * A zero-length transfer in PIO mode will not try to write/read
+	 * to/from the buffers
+	 *
+	 * REVISIT large transfers are exactly where we most want to be
+	 * using DMA.  If this happens much, split those transfers into
+	 * multiple DMA segments rather than forcing PIO.
+	 */
+	drv_data->dma_mapped = 0;
+	if (drv_data->len > 0 && drv_data->len <= MAX_DMA_LEN)
+		drv_data->dma_mapped = map_dma_buffers(drv_data);
+	if (drv_data->dma_mapped) {
 
 		/* Ensure we have the correct interrupt handler */
 		drv_data->transfer_handler = dma_transfer;

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Is it possible that the GPIO CS patch merged first before this one? And the patch below includes actually multiple changes, which makes the merge not so straight-forward.

Ned, I think you have reviewed my patch, and will it be convenient for you to update this one against mine? I have a lot of other stuffs depending on that, so it will be appreciated if you do. Thanks.

- eric

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

I'd say no ... the bugfixes should go into 2.6.27, but your GPIO CS stuff is a feature/cleanup change so it's not a candidate for 2.6.27 (or 2.6.27.1 etc).

As you saw in my comments. But the main thing is
that his chipselect-related bugfixes conflicted
with your new feature code. :)

- Dave

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

I guess I don't have the whole picture. There must be other patches that support the gpio_ calls. If you are saying that some of those patches have already merged, or are farther up the chain, then I guess I should accommodate that.

Not convenient, but possible. That would prevent the patch from applying to earlier kernels. Because the patch fixes operational bugs, I think porting is important. Of course, the original patch can be offered to stable, and a new one submitted to the current kernel.

Ned Forrester                                       nforrester-/[email protected]
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

OK, Dave, I will reply to your comments tomorrow and I will see if I can split the patch in two. I was hoping not to do that as the patch is not very big, but I expect it can be done. Sigh.

Ned Forrester                                       nforrester-/[email protected]
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Include me in the loop so that I know where to rebase. Thanks.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

I'm working on it, (emergencies yesterday, sorry), but I have questions...

Am I right to assume that you could/should/would make that cc: after you add your Acked-by:

I don't expect that they would go back as far as 2.6.20, though individuals could do so if they are still using old kernels. I would expect the stable maintainers to apply this as far back as they choose.

As some of you know, but perhaps not Eric, I am creating a driver that is a major expansion of pxa2xx_spi.c, so I am trying to stay involved with pxa2xx_spi.c development to limit structural changes that might make my work more difficult. This may be a fool's errand, because my changes are so significant that they likely never will be accepted into the driver. I think I have preserved all the existing functionality, while adding the ability to operate in various formats, and as a slave (limited to a single chip on the bus with a predictable transfer pattern) at the maximum bus speed of 13Mbit/sec. This driver is finally working, as of last month, but is a wreck in terms of coding style, so it is not ready for consideration.

More about that some other time. I just thought it might be useful to state my intentions and reason for interest.

[snip]

I accept your other changes to my patch notes, but I disagree that the above paragraph is related to this patch. I will leave it out unless you strongly object.

Note that the driver does not specifically do anything to support using SSPFRM as chip select. The documentation says that if no cs_control routine is specified, then the driver assumes use of SSPFRM, but really it does nothing. The real assumption is that the user knows what's right, and has either hooked up (and is happy with) SSPFRM, or is using a (single) chip that doesn't care.

Possibly a change to Documentation/spi/pxa2xx should be made to highlight the ways in which use of SSPFRM differs from use of a GPIO. Maybe I will take a shot at that with this patch.

		if (next_msg)
			if (next_msg->spi != msg->spi)
				next_msg = NULL;

Ned Forrester                                       nforrester-/[email protected]
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Stuff happens. :)

Yeah, but I don't think that particular order matters much.

At one point we had discussed having something like an SSP/DMA engine driver, on top of which could live various protcols ... SPI, maybe I2S for Alsa-SOC (if AC97 isn't appropriate), data capture/streaming stuff (which ISTR was more your interest) ... like that?

OK.

Separate, please. I'll repost the relevant patch from Vernon ... that seems like it should become a part of that.

For "A && B", it's guaranteed that A runs before B; both values are evaluated as nonzero/true vs zero/false.

For "X & Y" it isn't ... but those values are bit masks not simple true/false, so they're not usually equivalent.

C has worked that way forever.

- Dave

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

I have a very narrow view at that moment, and my understanding of the kernel is especially narrow. I don't know enough about what you are suggesting to offer an opinion. It might work out. Yes, streaming data is my interest.

Interesting. OK. Vernon slipped a note or two about DMA length, related to one of the two patches you will get today, so maybe his would work for the SSPFRM issue, too.

Hmmm... OK. I guess I have spent the last 28yrs trying to avoid reliance on that for naught. :-)

Ned Forrester                                       nforrester-/[email protected]
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/