This is a combination of two patches.
Patch 1:
Thanks to Johannes Sjölund for reporting that the Bus Pirate init could not deal with a Bus Pirate which is already in binary Bitbang mode. This is caused by a combination of the slowness of the Bus Pirate, the slowness of USB and a fast serial port flush routine which just flushes the buffer contents and does not wait until data arrival stops.
Make the Bus Pirate init more robust by running the flush command 10 times with 1.5 ms delay in between.
This code development was sponsored by Mattias Mattsson. Thanks! Tested a few dozen times, should work reliably.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Patch 2: 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.
Not for merge. The Bus Pirate interface has not been finalized yet, and this patch should help testing if the interface works as designed. No significant speedups expected yet because the code still uses the old small block sizes.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-buspirate_newcommands/buspirate_spi.c =================================================================== --- flashrom-buspirate_newcommands/buspirate_spi.c (Revision 1130) +++ flashrom-buspirate_newcommands/buspirate_spi.c (Arbeitskopie) @@ -22,6 +22,7 @@ #include <string.h> #include <stdlib.h> #include <ctype.h> +#include <unistd.h> #include "flash.h" #include "chipdrivers.h" #include "programmer.h" @@ -45,6 +46,8 @@ #define sp_flush_incoming(...) 0 #endif
+static int buspirate_interface_version; + static int buspirate_sendrecv(unsigned char *buf, unsigned int writecnt, unsigned int readcnt) { int i, ret = 0; @@ -130,7 +133,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 v4.6 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; @@ -141,6 +150,20 @@ /* Read any response and discard it. */ sp_flush_incoming(); } + /* USB is slow. The Bus Pirate is even slower. Apparently the flush + * action above is too fast or too early. Some stuff still remains in + * the pipe after the flush above, and one additional flush is not + * sufficient either. Use a 1.5 ms delay inside the loop to make + * mostly sure that at least one USB frame had time to arrive. + * Looping only 5 times is not sufficient and causes the + * ocassional failure. + * Folding the delay into the loop above is not reliable either. + */ + for (i = 0; i < 10; i++) { + usleep(1500); + /* Read any response and discard it. */ + sp_flush_incoming(); + } /* Enter raw bitbang mode */ buf[0] = 0x00; ret = buspirate_sendrecv(buf, 1, 5); @@ -148,6 +171,8 @@ return ret; if (memcmp(buf, "BBIO", 4)) { msg_perr("Entering raw bitbang mode failed!\n"); + msg_pdbg("Got %02x%02x%02x%02x%02x\n", + buf[0], buf[1], buf[2], buf[3], buf[4]); return 1; } msg_pdbg("Raw bitbang mode version %c\n", buf[4]); @@ -159,8 +184,12 @@ /* Enter raw SPI mode */ buf[0] = 0x01; ret = buspirate_sendrecv(buf, 1, 4); + if (ret) + return ret; if (memcmp(buf, "SPI", 3)) { msg_perr("Entering raw SPI mode failed!\n"); + msg_pdbg("Got %02x%02x%02x%02x\n", + buf[0], buf[1], buf[2], buf[3]); return 1; } msg_pdbg("Raw SPI mode version %c\n", buf[3]); @@ -210,6 +239,34 @@ return 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_pdbg("SPI command set v2 not available, using old commands " + "present in firmware vX.Y or later\n"); + + /* 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; + } else { + msg_pdbg("Using SPI command set v2.\n"); + buspirate_interface_version = 2; + } + buses_supported = CHIP_BUSTYPE_SPI; spi_controller = SPI_CONTROLLER_BUSPIRATE;
@@ -251,12 +308,56 @@ 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. + */ + buf = realloc(buf, max(writecnt + 5, readcnt + 1)); + if (!buf) { + msg_perr("Out of memory!\n"); + exit(1); // -1 + } + + /* Combined SPI write/read. */ + buf[i++] = 0x04; + buf[i++] = (writecnt >> 8) & 0xff; + buf[i++] = writecnt & 0xff; + buf[i++] = (readcnt >> 8) & 0xff; + buf[i++] = readcnt & 0xff; + memcpy(buf + i, writearr, writecnt); + + ret = buspirate_sendrecv(buf, i + writecnt, 1 + readcnt); + + if (ret) { + msg_perr("Bus Pirate communication error!\n"); + return SPI_GENERIC_ERROR; + } + + if (buf[0] != 0x01) { + msg_perr("Protocol error while sending SPI write/read!\n"); + return SPI_GENERIC_ERROR; + } + + /* Skip Ack. */ + memcpy(readarr, buf + 1, readcnt); + + return ret; +} + +int buspirate_spi_send_command_v1(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 > 16 || readcnt > 16 || (readcnt + writecnt) > 16) return SPI_INVALID_LENGTH;
@@ -307,6 +408,17 @@ 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); Index: flashrom-buspirate_newcommands/serial.c =================================================================== --- flashrom-buspirate_newcommands/serial.c (Revision 1130) +++ flashrom-buspirate_newcommands/serial.c (Arbeitskopie) @@ -200,8 +200,10 @@ #else tmp = write(sp_fd, buf, writecnt); #endif - if (tmp == -1) + if (tmp == -1) { + msg_perr("Serial port write error!\n"); return 1; + } if (!tmp) msg_pdbg("Empty write\n"); writecnt -= tmp; @@ -221,8 +223,10 @@ #else tmp = read(sp_fd, buf, readcnt); #endif - if (tmp == -1) + if (tmp == -1) { + msg_perr("Serial port read error!\n"); return 1; + } if (!tmp) msg_pdbg("Empty read\n"); readcnt -= tmp;
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 4.6 and this patch should help testing if the interface works as designed. No significant speedups expected yet because the code still uses the old small block sizes.
I would love to see timing comparisons for read and write, though. Once with <4.6 firmware and once with >=4.6 firmware.
Side note: The buffer management (search for realloc) is horrible, and should probably be changed to avoid the realloc in most (all cases). OTOH, a static huge buffer is also not optimal. A grow-only buffer like in other drivers may be the best choice.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-buspirate_newcommands/buspirate_spi.c =================================================================== --- flashrom-buspirate_newcommands/buspirate_spi.c (Revision 1178) +++ flashrom-buspirate_newcommands/buspirate_spi.c (Arbeitskopie) @@ -46,6 +46,8 @@ #define sp_flush_incoming(...) 0 #endif
+static int buspirate_interface_version; + static int buspirate_sendrecv(unsigned char *buf, unsigned int writecnt, unsigned int readcnt) { int i, ret = 0; @@ -131,7 +133,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 v4.6 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; @@ -231,6 +239,34 @@ return 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_pdbg("SPI command set v2 not available, using old commands " + "present in firmware vX.Y or later\n"); + + /* 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; + } else { + msg_pdbg("Using SPI command set v2.\n"); + buspirate_interface_version = 2; + } + buses_supported = CHIP_BUSTYPE_SPI; spi_controller = SPI_CONTROLLER_BUSPIRATE;
@@ -272,12 +308,56 @@ 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. + */ + buf = realloc(buf, max(writecnt + 5, readcnt + 1)); + if (!buf) { + msg_perr("Out of memory!\n"); + exit(1); // -1 + } + + /* Combined SPI write/read. */ + buf[i++] = 0x04; + buf[i++] = (writecnt >> 8) & 0xff; + buf[i++] = writecnt & 0xff; + buf[i++] = (readcnt >> 8) & 0xff; + buf[i++] = readcnt & 0xff; + memcpy(buf + i, writearr, writecnt); + + ret = buspirate_sendrecv(buf, i + writecnt, 1 + readcnt); + + if (ret) { + msg_perr("Bus Pirate communication error!\n"); + return SPI_GENERIC_ERROR; + } + + if (buf[0] != 0x01) { + msg_perr("Protocol error while sending SPI write/read!\n"); + return SPI_GENERIC_ERROR; + } + + /* Skip Ack. */ + memcpy(readarr, buf + 1, readcnt); + + return ret; +} + +int buspirate_spi_send_command_v1(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 > 16 || readcnt > 16 || (readcnt + writecnt) > 16) return SPI_INVALID_LENGTH;
@@ -328,6 +408,17 @@ 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);
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 4.6. A 16x speedup is expected from this patch for all read operations, erase operations and for some write operations (chips with page write only).
I would love to see timing comparisons for read and write. Once with <4.6 firmware and once with >=4.6 firmware.
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.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-buspirate_newcommands/buspirate_spi.c =================================================================== --- flashrom-buspirate_newcommands/buspirate_spi.c (Revision 1181) +++ 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; @@ -131,7 +182,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 v4.6 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; @@ -231,6 +288,42 @@ return 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_pdbg("SPI command set v2 not available, using old commands " + "present in firmware <v4.6.\n"); + + /* 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; + } 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; spi_controller = SPI_CONTROLLER_BUSPIRATE;
@@ -267,73 +360,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); } Index: flashrom-buspirate_newcommands/spi.h =================================================================== --- flashrom-buspirate_newcommands/spi.h (Revision 1181) +++ flashrom-buspirate_newcommands/spi.h (Arbeitskopie) @@ -125,5 +125,6 @@ #define SPI_INVALID_LENGTH -4 #define SPI_FLASHROM_BUG -5 #define SPI_PROGRAMMER_ERROR -6 +#define OOM_ERROR -7
#endif /* !__SPI_H__ */
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
Patrick
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.
Side note: The speedup support was merged in firmware v5.5. I have to fix this in the changelog and the code comments.
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!
Regards, Carl-Daniel
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@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); }
On 04.10.2010 15:11, Carl-Daniel Hailfinger wrote:
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.
Buffer management patch split out, subject is [flashrom] [PATCH] Bus Pirate buffer management revamp
I will repost the speedup patch once the buffer management patch is in.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Patrick Georgi