Patchwork sst25l.c: fix multi-part messages with broken spi masters

login
register
mail settings
Submitter hartleys
Date April 29, 2010, 6:34 p.m.
Message ID <0D753D10438DA54287A00B0270842697636E305D4A@AUSP01VMBX24.collaborationhost.net>
Download mbox | patch
Permalink /patch/51304/
State Accepted
Commit 0ffe0ce36e07185c693e3ff06ab5b3b6c30780ee
Headers show

Comments

hartleys - April 29, 2010, 6:34 p.m.
Some SPI masters (ep93xx) have limitations when using the SFRMOUT
signal for the spi device chip select.  The SFRMOUT signal is
only asserted as long as the spi transmit fifo contains data.  As
soon as the last bit is clocked into the receive fifo it gets
deasserted.

The functions sst25l_status and sst25l_match_device use the API
function spi_write_then_read to write a command to the flash then
read the response back.  This API function creates a two part spi
message for the write then read.  When this message is transferred
the SFRMOUT signal ends up getting deasserted after the command
phase.  This causes the command to get aborted by the device so
the read phase returns invalid data.

By changing sst25l_status and sst25l_match_device to use a single
transfer synchronous message, the SFRMOUT signal stays asserted
during the entire message so the correct data always gets returned.

This change will have no effect on SPI masters which use a chip
select mechanism (GPIO's, etc.) which does stay asserted correctly.
As a bonus, the single transfer synchronous messages complete faster
than multi-part messages.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: Andre Renaud <andre@bluewatersys.com>
Cc: Ryan Mallon <ryan@bluewatersys.com>

---

This driver is needed on the ep93xx based EDB93xx dev boards to
access the SPI flash.

This patch replaces "sst25l.c: simplify reading the device ManID/DevID"
which was previously posted.
Artem Bityutskiy - May 5, 2010, 5:45 a.m.
On Thu, 2010-04-29 at 13:34 -0500, H Hartley Sweeten wrote:
> Some SPI masters (ep93xx) have limitations when using the SFRMOUT
> signal for the spi device chip select.  The SFRMOUT signal is
> only asserted as long as the spi transmit fifo contains data.  As
> soon as the last bit is clocked into the receive fifo it gets
> deasserted.
> 
> The functions sst25l_status and sst25l_match_device use the API
> function spi_write_then_read to write a command to the flash then
> read the response back.  This API function creates a two part spi
> message for the write then read.  When this message is transferred
> the SFRMOUT signal ends up getting deasserted after the command
> phase.  This causes the command to get aborted by the device so
> the read phase returns invalid data.
> 
> By changing sst25l_status and sst25l_match_device to use a single
> transfer synchronous message, the SFRMOUT signal stays asserted
> during the entire message so the correct data always gets returned.
> 
> This change will have no effect on SPI masters which use a chip
> select mechanism (GPIO's, etc.) which does stay asserted correctly.
> As a bonus, the single transfer synchronous messages complete faster
> than multi-part messages.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Artem Bityutskiy <dedekind1@gmail.com>
> Cc: Andre Renaud <andre@bluewatersys.com>
> Cc: Ryan Mallon <ryan@bluewatersys.com>

Pushed to l2-mtd-2.6 / dunno.

But please, notice for future:
1. It is much nicer to put something like [PATCH v2] when you re-send.
2. In MTD we prefix everything with mtd:

Patch

diff --git a/drivers/mtd/devices/sst25l.c b/drivers/mtd/devices/sst25l.c
index fe17054..7de3bee 100644
--- a/drivers/mtd/devices/sst25l.c
+++ b/drivers/mtd/devices/sst25l.c
@@ -73,15 +73,25 @@  static struct flash_info __initdata sst25l_flash_info[] = {
 
 static int sst25l_status(struct sst25l_flash *flash, int *status)
 {
-	unsigned char command, response;
+	struct spi_message m;
+	struct spi_transfer t;
+	unsigned char cmd_resp[2];
 	int err;
 
-	command = SST25L_CMD_RDSR;
-	err = spi_write_then_read(flash->spi, &command, 1, &response, 1);
+	spi_message_init(&m);
+	memset(&t, 0, sizeof(struct spi_transfer));
+
+	cmd_resp[0] = SST25L_CMD_RDSR;
+	cmd_resp[1] = 0xff;
+	t.tx_buf = cmd_resp;
+	t.rx_buf = cmd_resp;
+	t.len = sizeof(cmd_resp);
+	spi_message_add_tail(&t, &m);
+	err = spi_sync(flash->spi, &m);
 	if (err < 0)
 		return err;
 
-	*status = response;
+	*status = cmd_resp[1];
 	return 0;
 }
 
@@ -328,33 +338,32 @@  out:
 static struct flash_info *__init sst25l_match_device(struct spi_device *spi)
 {
 	struct flash_info *flash_info = NULL;
-	unsigned char command[4], response;
+	struct spi_message m;
+	struct spi_transfer t;
+	unsigned char cmd_resp[6];
 	int i, err;
 	uint16_t id;
 
-	command[0] = SST25L_CMD_READ_ID;
-	command[1] = 0;
-	command[2] = 0;
-	command[3] = 0;
-	err = spi_write_then_read(spi, command, sizeof(command), &response, 1);
-	if (err < 0) {
-		dev_err(&spi->dev, "error reading device id msb\n");
-		return NULL;
-	}
-
-	id = response << 8;
-
-	command[0] = SST25L_CMD_READ_ID;
-	command[1] = 0;
-	command[2] = 0;
-	command[3] = 1;
-	err = spi_write_then_read(spi, command, sizeof(command), &response, 1);
+	spi_message_init(&m);
+	memset(&t, 0, sizeof(struct spi_transfer));
+
+	cmd_resp[0] = SST25L_CMD_READ_ID;
+	cmd_resp[1] = 0;
+	cmd_resp[2] = 0;
+	cmd_resp[3] = 0;
+	cmd_resp[4] = 0xff;
+	cmd_resp[5] = 0xff;
+	t.tx_buf = cmd_resp;
+	t.rx_buf = cmd_resp;
+	t.len = sizeof(cmd_resp);
+	spi_message_add_tail(&t, &m);
+	err = spi_sync(spi, &m);
 	if (err < 0) {
-		dev_err(&spi->dev, "error reading device id lsb\n");
+		dev_err(&spi->dev, "error reading device id\n");
 		return NULL;
 	}
 
-	id |= response;
+	id = (cmd_resp[4] << 8) | cmd_resp[5];
 
 	for (i = 0; i < ARRAY_SIZE(sst25l_flash_info); i++)
 		if (sst25l_flash_info[i].device_id == id)