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)