[flashrom] [PATCH] Make Bus Pirate init more robust, speed up flashing

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Mon Oct 4 15:11:18 CEST 2010


New patch. The corruption issue still needs to be debugged.
Should I split off the buffer management changes from the new interface
and send two separate patches? The buffer management part should be safe
to apply.

On 27.09.2010 11:06, Patrick Georgi wrote:
> Am 26.09.2010 23:20, schrieb Carl-Daniel Hailfinger:
>   
>> I would love to see timing comparisons for read and write. Once
>> with <4.6 firmware and once with >=4.6 firmware.
>>     
> v5.6 firmware pre this patch: 512kb read in 12mins 6secs.
> v5.6 firmware post this patch: 512kb read in 1min 57secs.
>
> Unfortunately, reading leads to corrupted images (and I don't know what
> writing does - it's untested for now). Since I'm confident that the
> issue is in the buspirate firmware, not in flashrom, I reported it to
> them: http://dangerousprototypes.com/forum/index.php?topic=997.0
>   

Thanks to Ian Lesnet for adding a new SPI mode to the Bus Pirate which
is specifically designed for flashrom. It has the potential to speed up
reads and writes a lot.
This patch implements flashrom support for the new SPI mode in a
hopefully backward compatible way.

The new Bus Pirate interface is present since Bus Pirate firmware
version 5.4.
An 8x speedup is expected from this patch for all read operations,
erase operations and for some write operations (chips with page write
only).

The buffer management of all Bus Pirate driver variants has been
revamped to use grow-only buffers with a reasonable initial default
size so realloc() will not have to be called in normal operation.
A side effect is the ability to switch to a static buffer without
major hassle.
Handle OOM gracefully.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
Index: flashrom-buspirate_newcommands/flash.h
===================================================================
--- flashrom-buspirate_newcommands/flash.h	(Revision 1184)
+++ flashrom-buspirate_newcommands/flash.h	(Arbeitskopie)
@@ -35,6 +35,9 @@
 
 #define ERROR_PTR ((void*)-1)
 
+/* Error codes */
+#define OOM_ERROR	-100
+
 typedef unsigned long chipaddr;
 
 int register_shutdown(void (*function) (void *data), void *data);
Index: flashrom-buspirate_newcommands/buspirate_spi.c
===================================================================
--- flashrom-buspirate_newcommands/buspirate_spi.c	(Revision 1184)
+++ flashrom-buspirate_newcommands/buspirate_spi.c	(Arbeitskopie)
@@ -46,6 +46,57 @@
 #define sp_flush_incoming(...) 0
 #endif
 
+static int buspirate_interface_version;
+static unsigned char *bp_commbuf = NULL;
+static int bp_commbufsize = 0;
+int bp_chunksize;
+
+static int buspirate_commbuf_resize(int bufsize)
+{
+	unsigned char *tmpbuf;
+
+	/* Never shrink. realloc() calls are expensive. */
+	if (bufsize <= bp_commbufsize)
+		return 0;
+
+	tmpbuf = realloc(bp_commbuf, bufsize);
+	if (!tmpbuf) {
+		/* This is debatable. Do we really want to free the existing
+		 * buffer or do we want to keep it around, especially if memory
+		 * is already tight?
+		 */
+		free(bp_commbuf);
+		bp_commbuf = NULL;
+		bp_commbufsize = 0;
+		msg_perr("Out of memory!\n");
+		return OOM_ERROR;
+	}
+
+	bp_commbuf = tmpbuf;
+	bp_commbufsize = bufsize;
+	return 0;
+}
+
+static int buspirate_commbuf_init(int bufsize)
+{
+	bp_commbuf = malloc(bufsize);
+	if (!bp_commbuf) {
+		bp_commbufsize = 0;
+		msg_perr("Out of memory!\n");
+		return OOM_ERROR;
+	}
+
+	bp_commbufsize = bufsize;
+	return 0;
+}
+
+static void buspirate_commbuf_shutdown(void)
+{
+	free(bp_commbuf);
+	bp_commbuf = NULL;
+	bp_commbufsize = 0;
+}
+
 static int buspirate_sendrecv(unsigned char *buf, unsigned int writecnt, unsigned int readcnt)
 {
 	int i, ret = 0;
@@ -94,6 +145,53 @@
 	{NULL,		0x0}
 };
 
+int buspirate_spi_set_config(unsigned char *buf, int spispeed)
+{
+	int ret;
+
+	/* Initial setup (SPI peripherals config): Enable power, CS high, AUX */
+	buf[0] = 0x40 | 0xb;
+	ret = buspirate_sendrecv(buf, 1, 1);
+	if (ret)
+		return 1;
+	if (buf[0] != 0x01) {
+		msg_perr("Protocol error while setting power/CS/AUX!\n");
+		return 1;
+	}
+
+	/* Set SPI speed */
+	buf[0] = 0x60 | spispeed;
+	ret = buspirate_sendrecv(buf, 1, 1);
+	if (ret)
+		return 1;
+	if (buf[0] != 0x01) {
+		msg_perr("Protocol error while setting SPI speed!\n");
+		return 1;
+	}
+	
+	/* Set SPI config: output type, idle, clock edge, sample */
+	buf[0] = 0x80 | 0xa;
+	ret = buspirate_sendrecv(buf, 1, 1);
+	if (ret)
+		return 1;
+	if (buf[0] != 0x01) {
+		msg_perr("Protocol error while setting SPI config!\n");
+		return 1;
+	}
+
+	/* De-assert CS# */
+	buf[0] = 0x03;
+	ret = buspirate_sendrecv(buf, 1, 1);
+	if (ret)
+		return 1;
+	if (buf[0] != 0x01) {
+		msg_perr("Protocol error while raising CS#!\n");
+		return 1;
+	}
+
+	return 0;
+}
+
 int buspirate_spi_init(void)
 {
 	unsigned char buf[512];
@@ -131,7 +229,13 @@
 		return ret;
 	free(dev);
 
-	/* This is the brute force version, but it should work. */
+	/* This is the brute force version, but it should work.
+	 * It is guaranteed to fail if a previous flashrom run was aborted
+	 * during a write with the new SPI commands in firmware v5.4 because
+	 * that firmware may wait for up to 4096 bytes of input before
+	 * responding to 0x00 again. The obvious workaround may cause startup
+	 * penalties of more than one second.
+	 */
 	for (i = 0; i < 19; i++) {
 		/* Enter raw bitbang mode */
 		buf[0] = 0x00;
@@ -191,44 +295,47 @@
 		return 1;
 	}
 
-	/* Initial setup (SPI peripherals config): Enable power, CS high, AUX */
-	buf[0] = 0x40 | 0xb;
-	ret = buspirate_sendrecv(buf, 1, 1);
-	if (ret)
+	if (buspirate_spi_set_config(buf, spispeed))
 		return 1;
-	if (buf[0] != 0x01) {
-		msg_perr("Protocol error while setting power/CS/AUX!\n");
-		return 1;
-	}
 
-	/* Set SPI speed */
-	buf[0] = 0x60 | spispeed;
-	ret = buspirate_sendrecv(buf, 1, 1);
+	/* Test combined SPI write/read, length 0. */
+	buf[0] = 0x04;
+	buf[1] = 0;
+	buf[2] = 0;
+	buf[3] = 0;
+	buf[4] = 0;
+	ret = buspirate_sendrecv(buf, 5, 1);
 	if (ret)
 		return 1;
 	if (buf[0] != 0x01) {
-		msg_perr("Protocol error while setting SPI speed!\n");
-		return 1;
-	}
-	
-	/* Set SPI config: output type, idle, clock edge, sample */
-	buf[0] = 0x80 | 0xa;
-	ret = buspirate_sendrecv(buf, 1, 1);
-	if (ret)
-		return 1;
-	if (buf[0] != 0x01) {
-		msg_perr("Protocol error while setting SPI config!\n");
-		return 1;
-	}
+		msg_pdbg("SPI command set v2 not available, using old commands "
+			 "present in firmware <v5.5.\n");
 
-	/* De-assert CS# */
-	buf[0] = 0x03;
-	ret = buspirate_sendrecv(buf, 1, 1);
-	if (ret)
-		return 1;
-	if (buf[0] != 0x01) {
-		msg_perr("Protocol error while raising CS#!\n");
-		return 1;
+		/* FIXME: Check the error code? */
+		/* We sent 4 bytes of 0x00, so we expect 4 BBIO1 responses. */
+		buspirate_sendrecv(buf, 0, 4 * 5);
+
+		/* Enter raw SPI mode again. */
+		buf[0] = 0x01;
+		/* FIXME: Check the error code? */
+		buspirate_sendrecv(buf, 1, 4);
+
+		buspirate_interface_version = 1;
+		/* Sensible default buffer size. */
+		if (buspirate_commbuf_init(16 + 3))
+			return OOM_ERROR;
+		bp_chunksize = 12;
+
+		/* Reinit the whole shebang. */
+		if (buspirate_spi_set_config(buf, spispeed))
+			return 1;
+	} else {
+		msg_pdbg("Using SPI command set v2.\n"); 
+		buspirate_interface_version = 2;
+		/* Sensible default buffer size. */
+		if (buspirate_commbuf_init(260 + 5))
+			return OOM_ERROR;
+		bp_chunksize = 256;
 	}
 
 	buses_supported = CHIP_BUSTYPE_SPI;
@@ -267,73 +374,123 @@
 	ret = serialport_shutdown();
 	if (ret)
 		return ret;
+
+	buspirate_commbuf_shutdown();
+
 	msg_pdbg("Bus Pirate shutdown completed.\n");
 
 	return 0;
 }
 
-int buspirate_spi_send_command(unsigned int writecnt, unsigned int readcnt,
+int buspirate_spi_send_command_v2(unsigned int writecnt, unsigned int readcnt,
 		const unsigned char *writearr, unsigned char *readarr)
 {
-	static unsigned char *buf = NULL;
 	int i = 0, ret = 0;
 
+	if (writecnt > 4096 || readcnt > 4096 || (readcnt + writecnt) > 4096)
+		return SPI_INVALID_LENGTH;
+
+	/* 5 bytes extra for command, writelen, readlen.
+	 * 1 byte extra for Ack/Nack.
+	 */
+	if (buspirate_commbuf_resize(max(writecnt + 5, readcnt + 1)))
+		return OOM_ERROR;
+
+	/* Combined SPI write/read. */
+	bp_commbuf[i++] = 0x04;
+	bp_commbuf[i++] = (writecnt >> 8) & 0xff;
+	bp_commbuf[i++] = writecnt & 0xff;
+	bp_commbuf[i++] = (readcnt >> 8) & 0xff;
+	bp_commbuf[i++] = readcnt & 0xff;
+	memcpy(bp_commbuf + i, writearr, writecnt);
+	
+	ret = buspirate_sendrecv(bp_commbuf, i + writecnt, 1 + readcnt);
+
+	if (ret) {
+		msg_perr("Bus Pirate communication error!\n");
+		return SPI_GENERIC_ERROR;
+	}
+
+	if (bp_commbuf[0] != 0x01) {
+		msg_perr("Protocol error while sending SPI write/read!\n");
+		return SPI_GENERIC_ERROR;
+	}
+
+	/* Skip Ack. */
+	memcpy(readarr, bp_commbuf + 1, readcnt);
+
+	return ret;
+}
+
+int buspirate_spi_send_command_v1(unsigned int writecnt, unsigned int readcnt,
+		const unsigned char *writearr, unsigned char *readarr)
+{
+	int i = 0, ret = 0;
+
 	if (writecnt > 16 || readcnt > 16 || (readcnt + writecnt) > 16)
 		return SPI_INVALID_LENGTH;
 
 	/* 3 bytes extra for CS#, len, CS#. */
-	buf = realloc(buf, writecnt + readcnt + 3);
-	if (!buf) {
-		msg_perr("Out of memory!\n");
-		exit(1); // -1
-	}
+	if (buspirate_commbuf_resize(writecnt + readcnt + 3))
+		return OOM_ERROR;
 
 	/* Assert CS# */
-	buf[i++] = 0x02;
+	bp_commbuf[i++] = 0x02;
 
-	buf[i++] = 0x10 | (writecnt + readcnt - 1);
-	memcpy(buf + i, writearr, writecnt);
+	bp_commbuf[i++] = 0x10 | (writecnt + readcnt - 1);
+	memcpy(bp_commbuf + i, writearr, writecnt);
 	i += writecnt;
-	memset(buf + i, 0, readcnt);
+	memset(bp_commbuf + i, 0, readcnt);
 
 	i += readcnt;
 	/* De-assert CS# */
-	buf[i++] = 0x03;
+	bp_commbuf[i++] = 0x03;
 
-	ret = buspirate_sendrecv(buf, i, i);
+	ret = buspirate_sendrecv(bp_commbuf, i, i);
 
 	if (ret) {
 		msg_perr("Bus Pirate communication error!\n");
 		return SPI_GENERIC_ERROR;
 	}
 
-	if (buf[0] != 0x01) {
+	if (bp_commbuf[0] != 0x01) {
 		msg_perr("Protocol error while lowering CS#!\n");
 		return SPI_GENERIC_ERROR;
 	}
 
-	if (buf[1] != 0x01) {
+	if (bp_commbuf[1] != 0x01) {
 		msg_perr("Protocol error while reading/writing SPI!\n");
 		return SPI_GENERIC_ERROR;
 	}
 
-	if (buf[i - 1] != 0x01) {
+	if (bp_commbuf[i - 1] != 0x01) {
 		msg_perr("Protocol error while raising CS#!\n");
 		return SPI_GENERIC_ERROR;
 	}
 
 	/* Skip CS#, length, writearr. */
-	memcpy(readarr, buf + 2 + writecnt, readcnt);
+	memcpy(readarr, bp_commbuf + 2 + writecnt, readcnt);
 
 	return ret;
 }
 
+int buspirate_spi_send_command(unsigned int writecnt, unsigned int readcnt,
+		const unsigned char *writearr, unsigned char *readarr)
+{
+	switch (buspirate_interface_version) {
+	case 2:
+		return buspirate_spi_send_command_v2(writecnt, readcnt, writearr, readarr);
+	default:
+		return buspirate_spi_send_command_v1(writecnt, readcnt, writearr, readarr);
+	}
+}
+
 int buspirate_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len)
 {
-	return spi_read_chunked(flash, buf, start, len, 12);
+	return spi_read_chunked(flash, buf, start, len, bp_chunksize);
 }
 
 int buspirate_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
 {
-	return spi_write_chunked(flash, buf, start, len, 12);
+	return spi_write_chunked(flash, buf, start, len, bp_chunksize);
 }


-- 
http://www.hailfinger.org/





More information about the flashrom mailing list