Change the SPI bitbanging core to fix a few subtle bugs (which had no effect so far) and to make integration of the RayeR SPIPGM and Nvidia MCP6x/MCP7x SPI patches easier.
A big thank you to Johannes Sjölund for testing the code as part of the Nvidia MCP6x/MCP7x SPI bitbanging patch.
This is the common part of the RayeR/Nvidia SPI patch. Tested on a dozen machines, works fine.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-bitbang_spi_fixup/flash.h =================================================================== --- flashrom-bitbang_spi_fixup/flash.h (Revision 1082) +++ flashrom-bitbang_spi_fixup/flash.h (Arbeitskopie) @@ -133,8 +133,6 @@
extern const int bitbang_spi_master_count;
-extern enum bitbang_spi_master bitbang_spi_master; - struct bitbang_spi_master_entry { void (*set_cs) (int val); void (*set_sck) (int val); @@ -535,7 +533,7 @@ /* bitbang_spi.c */ extern int bitbang_spi_half_period; extern const struct bitbang_spi_master_entry bitbang_spi_master_table[]; -int bitbang_spi_init(void); +int bitbang_spi_init(enum bitbang_spi_master master); int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len); int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len); Index: flashrom-bitbang_spi_fixup/hwaccess.h =================================================================== --- flashrom-bitbang_spi_fixup/hwaccess.h (Revision 1082) +++ flashrom-bitbang_spi_fixup/hwaccess.h (Arbeitskopie) @@ -176,6 +176,10 @@ #define __DARWIN__ #endif
+/* Clarification about OUTB/OUTW/OUTL argument order: + * OUT[BWL](val, port) + */ + #if defined(__FreeBSD__) || defined(__DragonFly__) #include <machine/cpufunc.h> #define off64_t off_t Index: flashrom-bitbang_spi_fixup/bitbang_spi.c =================================================================== --- flashrom-bitbang_spi_fixup/bitbang_spi.c (Revision 1082) +++ flashrom-bitbang_spi_fixup/bitbang_spi.c (Arbeitskopie) @@ -26,8 +26,8 @@ #include "chipdrivers.h" #include "spi.h"
-/* Length of half a clock period in usecs */ -int bitbang_spi_half_period = 0; +/* Length of half a clock period in usecs. Default to 1 (500 kHz). */ +int bitbang_spi_half_period = 1;
enum bitbang_spi_master bitbang_spi_master = BITBANG_SPI_INVALID;
@@ -37,6 +37,7 @@
const int bitbang_spi_master_count = ARRAY_SIZE(bitbang_spi_master_table);
+/* Note that CS# is active low, so val=0 means the chip is active. */ void bitbang_spi_set_cs(int val) { bitbang_spi_master_table[bitbang_spi_master].set_cs(val); @@ -57,10 +58,18 @@ return bitbang_spi_master_table[bitbang_spi_master].get_miso(); }
-int bitbang_spi_init(void) +int bitbang_spi_init(enum bitbang_spi_master master) { + bitbang_spi_master = master; + + if (bitbang_spi_master == BITBANG_SPI_INVALID) { + msg_perr("Invalid bitbang SPI master. \n" + "Please report a bug at flashrom@flashrom.org\n"); + return 1; + } bitbang_spi_set_cs(1); bitbang_spi_set_sck(0); + bitbang_spi_set_mosi(0); buses_supported = CHIP_BUSTYPE_SPI; return 0; } @@ -87,6 +96,7 @@ { static unsigned char *bufout = NULL; static unsigned char *bufin = NULL; + unsigned char *tmp; static int oldbufsize = 0; int bufsize; int i; @@ -98,20 +108,34 @@ bufsize = max(writecnt + readcnt, 260); /* Never shrink. realloc() calls are expensive. */ if (bufsize > oldbufsize) { - bufout = realloc(bufout, bufsize); - if (!bufout) { + tmp = realloc(bufout, bufsize); + if (!tmp) { msg_perr("Out of memory!\n"); + if (bufout) + free(bufout); + bufout = NULL; if (bufin) free(bufin); + bufin = NULL; + oldbufsize = 0; exit(1); - } - bufin = realloc(bufout, bufsize); - if (!bufin) { + } else + bufout = tmp; + + tmp = realloc(bufin, bufsize); + if (!tmp) { msg_perr("Out of memory!\n"); + if (bufin) + free(bufin); + bufin = NULL; if (bufout) free(bufout); + bufout = NULL; + oldbufsize = 0; exit(1); - } + } else + bufin = tmp; + oldbufsize = bufsize; } @@ -135,8 +159,13 @@
int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len) { - /* Maximum read length is unlimited, use 64k bytes. */ - return spi_read_chunked(flash, buf, start, len, 64 * 1024); + /* Maximum read length is unlimited in theory. + * The current implementation can handle reads of up to 65536 bytes. + * Please note that you need two buffers of 2n+4 bytes each for a read + * of n bytes, resulting in a total memory requirement of 4n+8 bytes. + * To conserve memory, read in chunks of 256 bytes. + */ + return spi_read_chunked(flash, buf, start, len, 256); }
int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
Am Freitag, den 16.07.2010, 23:54 +0200 schrieb Carl-Daniel Hailfinger:
-extern enum bitbang_spi_master bitbang_spi_master;
[...]
-int bitbang_spi_init(void); +int bitbang_spi_init(enum bitbang_spi_master master);
Eliminating global variables... great.
+/* Clarification about OUTB/OUTW/OUTL argument order:
- OUT[BWL](val, port)
- */
Looks unrelated, good hint though. Both argument orders are established in different PC compilers.
-/* Length of half a clock period in usecs */ -int bitbang_spi_half_period = 0; +/* Length of half a clock period in usecs. Default to 1 (500 kHz). */ +int bitbang_spi_half_period = 1;
Why so slow? SPI was designed to be fast (if you want a slow system, use I2C instead ;) ). It might be quite sensible on programmers at the parallel port having some cable length and no driver chip at the flash piece (maybe that applies to SPIPGM), but that is specific to this programmer. nVidia on-board stuff should run slow enough without any programmer_delay calls.
+/* Note that CS# is active low, so val=0 means the chip is active. */ void bitbang_spi_set_cs(int val)
I don't like the name spi_set_cs then, something like spi_set_negcs? To bad "/" and "#" characters are forbidden in identifiers...
bitbang_spi_set_cs(1); bitbang_spi_set_sck(0);
- bitbang_spi_set_mosi(0);
Initializeing all bits might make sense, looks OK.
buses_supported = CHIP_BUSTYPE_SPI;
Why do you take away LPC/FWH/PARALLEL here? Think nForce SPI, which could cooperate with nForce LPC just like on Intel chipsets.
/* Never shrink. realloc() calls are expensive. */ if (bufsize > oldbufsize) {
bufout = realloc(bufout, bufsize);if (!bufout) {
tmp = realloc(bufout, bufsize);if (!tmp) { msg_perr("Out of memory!\n");if (bufout)free(bufout);bufout = NULL; if (bufin) free(bufin);bufin = NULL;oldbufsize = 0; exit(1);
We can (somehow) handle Intel chipsets rejecting instructions or not being able to execute some instruction without exit()ing. Why do we need to call exit here?
Uncommented parts OK.
Regards, Michael Karcher
On 17.07.2010 00:18, Michael Karcher wrote:
Am Freitag, den 16.07.2010, 23:54 +0200 schrieb Carl-Daniel Hailfinger:
-extern enum bitbang_spi_master bitbang_spi_master;
[...]
-int bitbang_spi_init(void); +int bitbang_spi_init(enum bitbang_spi_master master);
Eliminating global variables... great.
I also made it static now.
-/* Length of half a clock period in usecs */ -int bitbang_spi_half_period = 0; +/* Length of half a clock period in usecs. Default to 1 (500 kHz). */ +int bitbang_spi_half_period = 1;
Why so slow? SPI was designed to be fast (if you want a slow system, use I2C instead ;) ). It might be quite sensible on programmers at the parallel port having some cable length and no driver chip at the flash piece (maybe that applies to SPIPGM), but that is specific to this programmer. nVidia on-board stuff should run slow enough without any programmer_delay calls.
Not sure. I figured that 1 usec would be a sane default, and could be reduced to zero by individual programmers depending on their knowledge about timing.
+/* Note that CS# is active low, so val=0 means the chip is active. */ void bitbang_spi_set_cs(int val)
I don't like the name spi_set_cs then, something like spi_set_negcs? To bad "/" and "#" characters are forbidden in identifiers...
Nobody besides SPI bitbanging driver writers will ever see that, and I bet they all are happy about not having to care.
bitbang_spi_set_cs(1); bitbang_spi_set_sck(0);
- bitbang_spi_set_mosi(0);
Initializeing all bits might make sense, looks OK.
Paranoid, I know.
buses_supported = CHIP_BUSTYPE_SPI;
Why do you take away LPC/FWH/PARALLEL here? Think nForce SPI, which could cooperate with nForce LPC just like on Intel chipsets.
You have a point. The bitbanging core now does not mess with buses_supported anymore.
/* Never shrink. realloc() calls are expensive. */ if (bufsize > oldbufsize) {
bufout = realloc(bufout, bufsize);if (!bufout) {
tmp = realloc(bufout, bufsize);if (!tmp) { msg_perr("Out of memory!\n");if (bufout)free(bufout);bufout = NULL; if (bufin) free(bufin);bufin = NULL;oldbufsize = 0; exit(1);We can (somehow) handle Intel chipsets rejecting instructions or not being able to execute some instruction without exit()ing. Why do we need to call exit here?
Failed memory allocation pretty much everywhere leads to exit(1). Changed to return an error code instead, and just keep the old buffer. That is saner anyway.
Uncommented parts OK.
Thanks for the review.
Change the SPI bitbanging core to fix a few subtle bugs (which had no effect so far) and to make integration of the RayeR SPIPGM and Nvidia MCP6x/MCP7x SPI patches easier. Handle out-of-memory gracefully instead of exiting the application.
A big thank you to Johannes Sjölund for testing the code as part of the Nvidia MCP6x/MCP7x SPI bitbanging patch.
This is the common part of the RayeR/Nvidia SPI patch. Tested on a dozen machines, works fine.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Index: flashrom-bitbang_spi_fixup/flash.h =================================================================== --- flashrom-bitbang_spi_fixup/flash.h (Revision 1083) +++ flashrom-bitbang_spi_fixup/flash.h (Arbeitskopie) @@ -133,8 +133,6 @@
extern const int bitbang_spi_master_count;
-extern enum bitbang_spi_master bitbang_spi_master; - struct bitbang_spi_master_entry { void (*set_cs) (int val); void (*set_sck) (int val); @@ -535,7 +533,7 @@ /* bitbang_spi.c */ extern int bitbang_spi_half_period; extern const struct bitbang_spi_master_entry bitbang_spi_master_table[]; -int bitbang_spi_init(void); +int bitbang_spi_init(enum bitbang_spi_master master); int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len); int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len); Index: flashrom-bitbang_spi_fixup/hwaccess.h =================================================================== --- flashrom-bitbang_spi_fixup/hwaccess.h (Revision 1083) +++ flashrom-bitbang_spi_fixup/hwaccess.h (Arbeitskopie) @@ -176,6 +176,10 @@ #define __DARWIN__ #endif
+/* Clarification about OUTB/OUTW/OUTL argument order: + * OUT[BWL](val, port) + */ + #if defined(__FreeBSD__) || defined(__DragonFly__) #include <machine/cpufunc.h> #define off64_t off_t Index: flashrom-bitbang_spi_fixup/bitbang_spi.c =================================================================== --- flashrom-bitbang_spi_fixup/bitbang_spi.c (Revision 1083) +++ flashrom-bitbang_spi_fixup/bitbang_spi.c (Arbeitskopie) @@ -26,10 +26,10 @@ #include "chipdrivers.h" #include "spi.h"
-/* Length of half a clock period in usecs */ -int bitbang_spi_half_period = 0; +/* Length of half a clock period in usecs. Default to 1 (500 kHz). */ +int bitbang_spi_half_period = 1;
-enum bitbang_spi_master bitbang_spi_master = BITBANG_SPI_INVALID; +static enum bitbang_spi_master bitbang_spi_master = BITBANG_SPI_INVALID;
const struct bitbang_spi_master_entry bitbang_spi_master_table[] = { {}, /* This entry corresponds to BITBANG_SPI_INVALID. */ @@ -37,6 +37,7 @@
const int bitbang_spi_master_count = ARRAY_SIZE(bitbang_spi_master_table);
+/* Note that CS# is active low, so val=0 means the chip is active. */ void bitbang_spi_set_cs(int val) { bitbang_spi_master_table[bitbang_spi_master].set_cs(val); @@ -57,11 +58,18 @@ return bitbang_spi_master_table[bitbang_spi_master].get_miso(); }
-int bitbang_spi_init(void) +int bitbang_spi_init(enum bitbang_spi_master master) { + bitbang_spi_master = master; + + if (bitbang_spi_master == BITBANG_SPI_INVALID) { + msg_perr("Invalid bitbang SPI master. \n" + "Please report a bug at flashrom@flashrom.org\n"); + return 1; + } bitbang_spi_set_cs(1); bitbang_spi_set_sck(0); - buses_supported = CHIP_BUSTYPE_SPI; + bitbang_spi_set_mosi(0); return 0; }
@@ -87,6 +95,7 @@ { static unsigned char *bufout = NULL; static unsigned char *bufin = NULL; + unsigned char *tmp; static int oldbufsize = 0; int bufsize; int i; @@ -98,20 +107,35 @@ bufsize = max(writecnt + readcnt, 260); /* Never shrink. realloc() calls are expensive. */ if (bufsize > oldbufsize) { - bufout = realloc(bufout, bufsize); - if (!bufout) { + tmp = realloc(bufout, bufsize); + if (!tmp) { msg_perr("Out of memory!\n"); - if (bufin) - free(bufin); - exit(1); - } - bufin = realloc(bufout, bufsize); - if (!bufin) { + /* Old buffer is untouched, so we can just return. */ + return SPI_GENERIC_ERROR; + } else + bufout = tmp; + + tmp = realloc(bufin, bufsize); + if (!tmp) { msg_perr("Out of memory!\n"); - if (bufout) - free(bufout); - exit(1); - } + /* Now we have a problem. bufout has the new size, but + * bufin still has the old size. Try to shrink bufout + * so both buffers have the same old size. + */ + tmp = realloc(bufout, oldbufsize); + if (!tmp) { + /* Not fatal, we're just using a few bytes more + * memory than necessary. + */ + msg_perr("Shrinking allocation failed!\n"); + } else { + bufout = tmp; + } + /* Old buffers are untouched, so we can just return. */ + return SPI_GENERIC_ERROR; + } else + bufin = tmp; + oldbufsize = bufsize; } @@ -135,8 +159,13 @@
int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len) { - /* Maximum read length is unlimited, use 64k bytes. */ - return spi_read_chunked(flash, buf, start, len, 64 * 1024); + /* Maximum read length is unlimited in theory. + * The current implementation can handle reads of up to 65536 bytes. + * Please note that you need two buffers of 2n+4 bytes each for a read + * of n bytes, resulting in a total memory requirement of 4n+8 bytes. + * To conserve memory, read in chunks of 256 bytes. + */ + return spi_read_chunked(flash, buf, start, len, 256); }
int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
Am Samstag, den 17.07.2010, 00:59 +0200 schrieb Carl-Daniel Hailfinger:
Why so slow? SPI was designed to be fast (if you want a slow system, use I2C instead ;) ). It might be quite sensible on programmers at the parallel port having some cable length and no driver chip at the flash piece (maybe that applies to SPIPGM), but that is specific to this programmer. nVidia on-board stuff should run slow enough without any programmer_delay calls.
Not sure. I figured that 1 usec would be a sane default, and could be reduced to zero by individual programmers depending on their knowledge about timing.
My idea was that everything that is not "broken" in hardware doesn't need a delay, so 0 is a sane default and programmers can set this to a bigger value in their code if they think that this programmer is so slow, but I don't really care, 1 as default is OK too.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
I'd rather kill the whole buffer stuff (see attached patch) before applying your patch (and of course remove the buffer stuff from your patch, too). No other reason not to ack.
Regards, Michael Karcher
On 17.07.2010 09:47, Michael Karcher wrote:
Am Samstag, den 17.07.2010, 00:59 +0200 schrieb Carl-Daniel Hailfinger:
Why so slow? SPI was designed to be fast (if you want a slow system, use I2C instead ;) ). It might be quite sensible on programmers at the parallel port having some cable length and no driver chip at the flash piece (maybe that applies to SPIPGM), but that is specific to this programmer. nVidia on-board stuff should run slow enough without any programmer_delay calls.
Not sure. I figured that 1 usec would be a sane default, and could be reduced to zero by individual programmers depending on their knowledge about timing.
My idea was that everything that is not "broken" in hardware doesn't need a delay, so 0 is a sane default and programmers can set this to a bigger value in their code if they think that this programmer is so slow, but I don't really care, 1 as default is OK too.
I based the delay requirement on the possibility of really fast hardware. Once your hardware can do bitbanging at speeds above 16 MHz, it is too fast for some SPI chips. If someone doesn't know the maximum hardware speed for a new driver, he/she can simply keep the default delay and be confident that it won't be too fast. For all I know, the Nvidia SPI stuff might reach a few MHz.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
I'd rather kill the whole buffer stuff (see attached patch) before applying your patch (and of course remove the buffer stuff from your patch, too). No other reason not to ack.
From 883e73a446a0fa2614fe4b6309a43ecfeeedea24 Mon Sep 17 00:00:00 2001
From: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de Date: Sat, 17 Jul 2010 09:43:58 +0200 Subject: [PATCH] remove temporary buffers from bitbanging
There is no point in memcpying data around in/out temporary buffers if the original buffer does as good as the temporary one.
Compile tested, no functional testing.
Signed-off-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net with one small change.
diff --git a/bitbang_spi.c b/bitbang_spi.c index 446be11..583db32 100644 --- a/bitbang_spi.c +++ b/bitbang_spi.c @@ -85,58 +85,24 @@ uint8_t bitbang_spi_readwrite_byte(uint8_t val) int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) {
static unsigned char *bufout = NULL;
static unsigned char *bufin = NULL;
static int oldbufsize = 0;
int bufsize; int i;
/* Arbitrary size limitation here. We're only constrained by memory. */
if (writecnt > 65536 || readcnt > 65536)
return SPI_INVALID_LENGTH;bufsize = max(writecnt + readcnt, 260);
/* Never shrink. realloc() calls are expensive. */
if (bufsize > oldbufsize) {
bufout = realloc(bufout, bufsize);if (!bufout) {msg_perr("Out of memory!\n");if (bufin)free(bufin);exit(1);}bufin = realloc(bufout, bufsize);if (!bufin) {msg_perr("Out of memory!\n");if (bufout)free(bufout);exit(1);}oldbufsize = bufsize;}
memcpy(bufout, writearr, writecnt);
/* Shift out 0x00 while reading data. */
memset(bufout + writecnt, 0x00, readcnt);
/* Make sure any non-read data is 0xff. */
memset(bufin + writecnt, 0xff, readcnt);
bitbang_spi_set_cs(0);
for (i = 0; i < readcnt + writecnt; i++) {
bufin[i] = bitbang_spi_readwrite_byte(bufout[i]);}
- for (i = 0; i < writecnt; i++)
bitbang_spi_readwrite_byte(writearr[i]);- for (i = 0; i < readcnt; i++)
readarr[i] = bitbang_spi_readwrite_byte(0);- programmer_delay(bitbang_spi_half_period); bitbang_spi_set_cs(1); programmer_delay(bitbang_spi_half_period);
memcpy(readarr, bufin + writecnt, readcnt);
return 0;
}
I have to admit that your no-extra-buffer solution is elegant and should work fine. It makes printing the readback during write impossible, but such a feature is only needed for debugging of new drivers anyway.
int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len) {
- /* Maximum read length is unlimited, use 64k bytes. */
- return spi_read_chunked(flash, buf, start, len, 64 * 1024);
- return spi_nbyte_read(start, buf, len);
Please undo that hunk. It breaks a few semi-supported SPI chips with odd sector sizes and is inconsistent with how the other SPI drivers do it.
}
int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
Regards, Carl-Daniel
Am Samstag, den 17.07.2010, 12:31 +0200 schrieb Carl-Daniel Hailfinger:
My idea was that everything that is not "broken" in hardware doesn't need a delay, so 0 is a sane default and programmers can set this to a bigger value in their code if they think that this programmer is so slow, but I don't really care, 1 as default is OK too.
I based the delay requirement on the possibility of really fast hardware. Once your hardware can do bitbanging at speeds above 16 MHz, it is too fast for some SPI chips.
I don't see any uncached Bus in PC environment reaching 16MHz anytime soon (i.e. 32 megacycles per second). That's why I expected the code to be always slow enough.
Please undo that hunk. It breaks a few semi-supported SPI chips with odd sector sizes and is inconsistent with how the other SPI drivers do it.
OK, undone,
and committed as r1084.
Regards, Michael Karcher
Am Samstag, den 17.07.2010, 00:59 +0200 schrieb Carl-Daniel Hailfinger:
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Rebase to r1084 and this is
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Regards, Michael Karcher
On 17.07.2010 12:47, Michael Karcher wrote:
Rebase to r1084 and this is
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Change the SPI bitbanging core to fix a subtle bug (which had no effect so far) and to make integration of the RayeR SPIPGM and Nvidia MCP6x/MCP7x SPI patches easier.
A big thank you to Johannes Sjölund for testing the code as part of the Nvidia MCP6x/MCP7x SPI bitbanging patch.
This is the common part of the RayeR/Nvidia SPI patch. Tested on a dozen machines, works fine.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-bitbang_spi_fixup/flash.h =================================================================== --- flashrom-bitbang_spi_fixup/flash.h (Revision 1084) +++ flashrom-bitbang_spi_fixup/flash.h (Arbeitskopie) @@ -133,8 +133,6 @@
extern const int bitbang_spi_master_count;
-extern enum bitbang_spi_master bitbang_spi_master; - struct bitbang_spi_master_entry { void (*set_cs) (int val); void (*set_sck) (int val); @@ -535,7 +533,7 @@ /* bitbang_spi.c */ extern int bitbang_spi_half_period; extern const struct bitbang_spi_master_entry bitbang_spi_master_table[]; -int bitbang_spi_init(void); +int bitbang_spi_init(enum bitbang_spi_master master); int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len); int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len); Index: flashrom-bitbang_spi_fixup/hwaccess.h =================================================================== --- flashrom-bitbang_spi_fixup/hwaccess.h (Revision 1084) +++ flashrom-bitbang_spi_fixup/hwaccess.h (Arbeitskopie) @@ -176,6 +176,10 @@ #define __DARWIN__ #endif
+/* Clarification about OUTB/OUTW/OUTL argument order: + * OUT[BWL](val, port) + */ + #if defined(__FreeBSD__) || defined(__DragonFly__) #include <machine/cpufunc.h> #define off64_t off_t Index: flashrom-bitbang_spi_fixup/bitbang_spi.c =================================================================== --- flashrom-bitbang_spi_fixup/bitbang_spi.c (Revision 1084) +++ flashrom-bitbang_spi_fixup/bitbang_spi.c (Arbeitskopie) @@ -26,10 +26,10 @@ #include "chipdrivers.h" #include "spi.h"
-/* Length of half a clock period in usecs */ -int bitbang_spi_half_period = 0; +/* Length of half a clock period in usecs. Default to 1 (500 kHz). */ +int bitbang_spi_half_period = 1;
-enum bitbang_spi_master bitbang_spi_master = BITBANG_SPI_INVALID; +static enum bitbang_spi_master bitbang_spi_master = BITBANG_SPI_INVALID;
const struct bitbang_spi_master_entry bitbang_spi_master_table[] = { {}, /* This entry corresponds to BITBANG_SPI_INVALID. */ @@ -37,6 +37,7 @@
const int bitbang_spi_master_count = ARRAY_SIZE(bitbang_spi_master_table);
+/* Note that CS# is active low, so val=0 means the chip is active. */ void bitbang_spi_set_cs(int val) { bitbang_spi_master_table[bitbang_spi_master].set_cs(val); @@ -57,11 +58,18 @@ return bitbang_spi_master_table[bitbang_spi_master].get_miso(); }
-int bitbang_spi_init(void) +int bitbang_spi_init(enum bitbang_spi_master master) { + bitbang_spi_master = master; + + if (bitbang_spi_master == BITBANG_SPI_INVALID) { + msg_perr("Invalid bitbang SPI master. \n" + "Please report a bug at flashrom@flashrom.org\n"); + return 1; + } bitbang_spi_set_cs(1); bitbang_spi_set_sck(0); - buses_supported = CHIP_BUSTYPE_SPI; + bitbang_spi_set_mosi(0); return 0; }
Change the SPI bitbanging core to fix a subtle bug (which had no effect so far) and to make integration of the RayeR SPIPGM and Nvidia MCP6x/MCP7x SPI patches easier. Kill a few global variables and require explicit initialization of bitbanging delay.
A big thank you to Johannes Sjölund for testing an earlier version of the code as part of the Nvidia MCP6x/MCP7x SPI bitbanging patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-bitbang_spi_fixup/flash.h =================================================================== --- flashrom-bitbang_spi_fixup/flash.h (Revision 1084) +++ flashrom-bitbang_spi_fixup/flash.h (Arbeitskopie) @@ -133,8 +133,6 @@
extern const int bitbang_spi_master_count;
-extern enum bitbang_spi_master bitbang_spi_master; - struct bitbang_spi_master_entry { void (*set_cs) (int val); void (*set_sck) (int val); @@ -533,9 +531,7 @@ int ft2232_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len);
/* bitbang_spi.c */ -extern int bitbang_spi_half_period; -extern const struct bitbang_spi_master_entry bitbang_spi_master_table[]; -int bitbang_spi_init(void); +int bitbang_spi_init(enum bitbang_spi_master master, int halfperiod); int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len); int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len); Index: flashrom-bitbang_spi_fixup/hwaccess.h =================================================================== --- flashrom-bitbang_spi_fixup/hwaccess.h (Revision 1084) +++ flashrom-bitbang_spi_fixup/hwaccess.h (Arbeitskopie) @@ -176,6 +176,10 @@ #define __DARWIN__ #endif
+/* Clarification about OUTB/OUTW/OUTL argument order: + * OUT[BWL](val, port) + */ + #if defined(__FreeBSD__) || defined(__DragonFly__) #include <machine/cpufunc.h> #define off64_t off_t Index: flashrom-bitbang_spi_fixup/bitbang_spi.c =================================================================== --- flashrom-bitbang_spi_fixup/bitbang_spi.c (Revision 1084) +++ flashrom-bitbang_spi_fixup/bitbang_spi.c (Arbeitskopie) @@ -26,10 +26,10 @@ #include "chipdrivers.h" #include "spi.h"
-/* Length of half a clock period in usecs */ -int bitbang_spi_half_period = 0; +/* Length of half a clock period in usecs. */ +static int bitbang_spi_half_period;
-enum bitbang_spi_master bitbang_spi_master = BITBANG_SPI_INVALID; +static enum bitbang_spi_master bitbang_spi_master = BITBANG_SPI_INVALID;
const struct bitbang_spi_master_entry bitbang_spi_master_table[] = { {}, /* This entry corresponds to BITBANG_SPI_INVALID. */ @@ -37,6 +37,7 @@
const int bitbang_spi_master_count = ARRAY_SIZE(bitbang_spi_master_table);
+/* Note that CS# is active low, so val=0 means the chip is active. */ void bitbang_spi_set_cs(int val) { bitbang_spi_master_table[bitbang_spi_master].set_cs(val); @@ -57,11 +58,19 @@ return bitbang_spi_master_table[bitbang_spi_master].get_miso(); }
-int bitbang_spi_init(void) +int bitbang_spi_init(enum bitbang_spi_master master, int halfperiod) { + bitbang_spi_master = master; + bitbang_spi_half_period = halfperiod; + + if (bitbang_spi_master == BITBANG_SPI_INVALID) { + msg_perr("Invalid bitbang SPI master. \n" + "Please report a bug at flashrom@flashrom.org\n"); + return 1; + } bitbang_spi_set_cs(1); bitbang_spi_set_sck(0); - buses_supported = CHIP_BUSTYPE_SPI; + bitbang_spi_set_mosi(0); return 0; }
New version. Kill even more globals.
Change the SPI bitbanging core to fix a subtle bug (which had no effect so far) and to make integration of the RayeR SPIPGM and Nvidia MCP6x/MCP7x SPI patches easier. Kill a few global variables and require explicit initialization of bitbanging delay.
A big thank you to Johannes Sjölund for testing an earlier version of the code as part of the Nvidia MCP6x/MCP7x SPI bitbanging patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-bitbang_spi_fixup/flash.h =================================================================== --- flashrom-bitbang_spi_fixup/flash.h (Revision 1084) +++ flashrom-bitbang_spi_fixup/flash.h (Arbeitskopie) @@ -133,8 +133,6 @@
extern const int bitbang_spi_master_count;
-extern enum bitbang_spi_master bitbang_spi_master; - struct bitbang_spi_master_entry { void (*set_cs) (int val); void (*set_sck) (int val); @@ -533,9 +531,7 @@ int ft2232_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len);
/* bitbang_spi.c */ -extern int bitbang_spi_half_period; -extern const struct bitbang_spi_master_entry bitbang_spi_master_table[]; -int bitbang_spi_init(void); +int bitbang_spi_init(enum bitbang_spi_master master, int halfperiod); int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len); int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len); Index: flashrom-bitbang_spi_fixup/hwaccess.h =================================================================== --- flashrom-bitbang_spi_fixup/hwaccess.h (Revision 1084) +++ flashrom-bitbang_spi_fixup/hwaccess.h (Arbeitskopie) @@ -176,6 +176,10 @@ #define __DARWIN__ #endif
+/* Clarification about OUTB/OUTW/OUTL argument order: + * OUT[BWL](val, port) + */ + #if defined(__FreeBSD__) || defined(__DragonFly__) #include <machine/cpufunc.h> #define off64_t off_t Index: flashrom-bitbang_spi_fixup/bitbang_spi.c =================================================================== --- flashrom-bitbang_spi_fixup/bitbang_spi.c (Revision 1084) +++ flashrom-bitbang_spi_fixup/bitbang_spi.c (Arbeitskopie) @@ -26,46 +26,55 @@ #include "chipdrivers.h" #include "spi.h"
-/* Length of half a clock period in usecs */ -int bitbang_spi_half_period = 0; +/* Length of half a clock period in usecs. */ +static int bitbang_spi_half_period;
-enum bitbang_spi_master bitbang_spi_master = BITBANG_SPI_INVALID; +static enum bitbang_spi_master bitbang_spi_master = BITBANG_SPI_INVALID;
-const struct bitbang_spi_master_entry bitbang_spi_master_table[] = { +static const struct bitbang_spi_master_entry bitbang_spi_master_table[] = { {}, /* This entry corresponds to BITBANG_SPI_INVALID. */ };
const int bitbang_spi_master_count = ARRAY_SIZE(bitbang_spi_master_table);
-void bitbang_spi_set_cs(int val) +/* Note that CS# is active low, so val=0 means the chip is active. */ +static void bitbang_spi_set_cs(int val) { bitbang_spi_master_table[bitbang_spi_master].set_cs(val); }
-void bitbang_spi_set_sck(int val) +static void bitbang_spi_set_sck(int val) { bitbang_spi_master_table[bitbang_spi_master].set_sck(val); }
-void bitbang_spi_set_mosi(int val) +static void bitbang_spi_set_mosi(int val) { bitbang_spi_master_table[bitbang_spi_master].set_mosi(val); }
-int bitbang_spi_get_miso(void) +static int bitbang_spi_get_miso(void) { return bitbang_spi_master_table[bitbang_spi_master].get_miso(); }
-int bitbang_spi_init(void) +int bitbang_spi_init(enum bitbang_spi_master master, int halfperiod) { + bitbang_spi_master = master; + bitbang_spi_half_period = halfperiod; + + if (bitbang_spi_master == BITBANG_SPI_INVALID) { + msg_perr("Invalid bitbang SPI master. \n" + "Please report a bug at flashrom@flashrom.org\n"); + return 1; + } bitbang_spi_set_cs(1); bitbang_spi_set_sck(0); - buses_supported = CHIP_BUSTYPE_SPI; + bitbang_spi_set_mosi(0); return 0; }
-uint8_t bitbang_spi_readwrite_byte(uint8_t val) +static uint8_t bitbang_spi_readwrite_byte(uint8_t val) { uint8_t ret = 0; int i;
Am Samstag, den 17.07.2010, 14:35 +0200 schrieb Carl-Daniel Hailfinger:
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
This patch as-is is Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de According to current flashrom desing I see no technical reason to not accept the patch.
BUT: We really should think about killing the SPI bitbang master table. Why not just pass a pointer to a "struct bitbang_spi_master_enty" to bitbang_spi_init which will then be stored in a static global pointer instead of a table index as it is done currently? Maybe it's the best time to do this change now before we merge the bit-bang drivers if this change is accepted.
Regards, Michael Karcher
On 17.07.2010 14:44, Michael Karcher wrote:
Am Samstag, den 17.07.2010, 14:35 +0200 schrieb Carl-Daniel Hailfinger:
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
This patch as-is is Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks, committed in r1085.
According to current flashrom desing I see no technical reason to not accept the patch.
BUT: We really should think about killing the SPI bitbang master table. Why not just pass a pointer to a "struct bitbang_spi_master_enty" to bitbang_spi_init which will then be stored in a static global pointer instead of a table index as it is done currently? Maybe it's the best time to do this change now before we merge the bit-bang drivers if this change is accepted.
This design question applies to all programmer related tables. Advantages: The current checks for miscompilation on startup would be obsolete, and the programmer enum and related variables would not be needed anymore either. Disadvantages: It will be a lot more difficult to blacklist certain actions based on the programmer (because that action is not supported) and that may require conversion of some error avoidance paths to error recovery paths.
Not sure. It looked good at a first glance, but now I have serious doubts.
Regards, Carl-Daniel
Am Samstag, den 17.07.2010, 15:02 +0200 schrieb Carl-Daniel Hailfinger:
This design question applies to all programmer related tables. Advantages: The current checks for miscompilation on startup would be obsolete, and the programmer enum and related variables would not be needed anymore either.
Right.
Disadvantages: It will be a lot more difficult to blacklist certain actions based on the programmer (because that action is not supported) and that may require conversion of some error avoidance paths to error recovery paths.
Not necessarily. You can leave the programmer enum alive, but *always* including all programmers present in source, so the ugly #ifdef SUPPORT_FOO // PROGRAMMER_FOO /// #endif mess goes away. Then you create a "programmer_id" field in the programmer structure which can be used to look up the programmer ID and compared to certain values.
Regards, Michael Karcher
-
Carl-Daniel Hailfinger -
Michael Karcher