I tried the latest svn version (6 May 09) of flashrom and unlike older versions didn't get a warning on my chipset. flashrom reads my chip successfully and outputs a fine Phoenix bios. After writing a new image into the chip I found that writer is not fully functional and reading the chip again results in an image that is neither original one nor the new image. then I tried erase functionality and it resulted in some 0xFF and some unchanged bytes in the chip. Currently writing either images doesn't change the chip and it remains in mostly 0xFF bytes.
Here are the flashrom detection info :
ali@Velocity:~/tmp/f$ sudo ./flashrom/flashrom Calibrating delay loop... OK. No coreboot table found. Found chipset "Intel ICH7M", enabling flash write... OK. Found chip "SST SST25VF080B" (1024 KB) at physical address 0xfff00000. No operations were specified.
verbose output at http://coreboot.pastebin.com/m604f01b6
history of my commands : http://coreboot.pastebin.com/m5c3c2372
Original Rom : http://filebin.ca/mhtqhm/mybios.rom
lspci -vv output attached.
I try to keep my Lenovo 3000 up and running and waiting for helps. :)
I'm also online on #coreboot as nadalizadeh
Thanks Ali Nadalizadeh
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hmm I checked the orig bios image for any special handling and there is none. Even it seems that EC has its own flash somewhere else. (this is good)
Perhaps there are some locks in chip itself? Any expert here on that? Can we read the status reg from the chip? The WP# of the chip seems just to enable/disable modifications to the to the BP bits.
I suspect the part flashing does not work well. There seems to be no obstacles elsewhere.
Can you post here lspci -xxx
Rudolf
lspci -xxx output attached
On Thu, May 7, 2009 at 12:21 AM, Rudolf Marek r.marek@assembler.cz wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hmm I checked the orig bios image for any special handling and there is none. Even it seems that EC has its own flash somewhere else. (this is good)
Perhaps there are some locks in chip itself? Any expert here on that? Can we read the status reg from the chip? The WP# of the chip seems just to enable/disable modifications to the to the BP bits.
I suspect the part flashing does not work well. There seems to be no obstacles elsewhere.
Can you post here lspci -xxx
Rudolf
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkoB6iQACgkQ3J9wPJqZRNUwgACfZkN1B45fPlmQ0jV0NdMtOvVC 0P4AoIA6tVBC/XzOdleoBL7amNpRpIHJ =RWFH -----END PGP SIGNATURE-----
-- Ali Nadalizadeh
Ali Nadalizadeh wrote:
I'm also online on #coreboot as nadalizadeh
Some analysis after Ali worked with Carl-Daniel and me to debug this.
The chip needs a write enable command to set it's write enable latch before each erase or write command.
flashrom sends this as a separate command, but for software sequenced ICH SPI that doesn't work, it needs to go into the ICH PREOP register and not be sent as an individual command.
Because this fails when trying to send an erase command, chip erase aborts. Ignoring that write enable error allows the erase to continue, and succeed.
This chip doesn't support writes of more than one byte at a time using the 02 command, but the flashrom ICH SPI driver (and maybe others) assumes that all chips support more data in one go. Many do.
Ali had to go, and we have to rest a little, right now the machine is running AAI programming, writing two bytes at a time.
If that doesn't work either, the next step is to try the 02 byte program command but actually send only a single byte at a time. (force maxdata=1 somewhere suitable)
More updates this afternoon. I think it'll work in the end.
//Peter
Until the ICH SPI driver can handle preopcodes as standalone opcodes, we should handle such special opcode failure gracefully on ICH and compatible chipsets.
This fixes chip erase on almost all ICH+VIA SPI masters.
Thanks to Ali Nadalizadeh for helping track down this bug!
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Patch inline and attached.
Index: flashrom-spi_write_enable_error_checking/it87spi.c =================================================================== --- flashrom-spi_write_enable_error_checking/it87spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/it87spi.c (Arbeitskopie) @@ -196,11 +196,16 @@ }
/* Page size is usually 256 bytes */ -static void it8716f_spi_page_program(int block, uint8_t *buf, uint8_t *bios) +static int it8716f_spi_page_program(int block, uint8_t *buf, uint8_t *bios) { int i; + int result;
- spi_write_enable(); + result = spi_write_enable(); + if (result) { + printf_debug("spi_write_enable failed\n"); + return result; + } OUTB(0x06, it8716f_flashport + 1); OUTB(((2 + (fast_spi ? 1 : 0)) << 4), it8716f_flashport); for (i = 0; i < 256; i++) { @@ -212,6 +217,7 @@ */ while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) usleep(1000); + return 0; }
/* @@ -222,12 +228,17 @@ { int total_size = 1024 * flash->total_size; int i; + int result;
fast_spi = 0;
spi_disable_blockprotect(); for (i = 0; i < total_size; i++) { - spi_write_enable(); + result = spi_write_enable(); + if (result) { + printf_debug("spi_write_enable failed\n"); + return result; + } spi_byte_program(i, buf[i]); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) myusec_delay(10); Index: flashrom-spi_write_enable_error_checking/spi.c =================================================================== --- flashrom-spi_write_enable_error_checking/spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/spi.c (Arbeitskopie) @@ -362,7 +362,16 @@ } result = spi_write_enable(); if (result) { - printf_debug("spi_write_enable failed\n"); + printf_debug("spi_write_enable failed"); + switch (flashbus) { + case BUS_TYPE_ICH7_SPI: + case BUS_TYPE_ICH9_SPI: + case BUS_TYPE_VIA_SPI: + printf_debug(" due to SPI master limitation, ignoring"); + break; + default: + printf_debug("\n"); + } return result; } /* Send CE (Chip Erase) */ @@ -392,8 +401,17 @@ } result = spi_write_enable(); if (result) { - printf_debug("spi_write_enable failed\n"); - return result; + printf_debug("spi_write_enable failed"); + switch (flashbus) { + case BUS_TYPE_ICH7_SPI: + case BUS_TYPE_ICH9_SPI: + case BUS_TYPE_VIA_SPI: + printf_debug(" due to SPI master limitation, ignoring"); + break; + default: + printf_debug("\n"); + } + //return result; } /* Send CE (Chip Erase) */ result = spi_command(sizeof(cmd), 0, cmd, NULL); @@ -424,11 +442,25 @@ int spi_block_erase_52(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_BE_52_OUTSIZE] = {JEDEC_BE_52}; + int result;
cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff); - spi_write_enable(); + result = spi_write_enable(); + if (result) { + printf_debug("spi_write_enable failed"); + switch (flashbus) { + case BUS_TYPE_ICH7_SPI: + case BUS_TYPE_ICH9_SPI: + case BUS_TYPE_VIA_SPI: + printf_debug(" due to SPI master limitation, ignoring"); + break; + default: + printf_debug("\n"); + } + return result; + } /* Send BE (Block Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared. @@ -447,11 +479,25 @@ int spi_block_erase_d8(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_BE_D8_OUTSIZE] = { JEDEC_BE_D8 }; + int result;
cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff); - spi_write_enable(); + result = spi_write_enable(); + if (result) { + printf_debug("spi_write_enable failed"); + switch (flashbus) { + case BUS_TYPE_ICH7_SPI: + case BUS_TYPE_ICH9_SPI: + case BUS_TYPE_VIA_SPI: + printf_debug(" due to SPI master limitation, ignoring"); + break; + default: + printf_debug("\n"); + } + return result; + } /* Send BE (Block Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared. @@ -489,11 +535,26 @@ int spi_sector_erase(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_SE_OUTSIZE] = { JEDEC_SE }; + int result; + cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff);
- spi_write_enable(); + result = spi_write_enable(); + if (result) { + printf_debug("spi_write_enable failed"); + switch (flashbus) { + case BUS_TYPE_ICH7_SPI: + case BUS_TYPE_ICH9_SPI: + case BUS_TYPE_VIA_SPI: + printf_debug(" due to SPI master limitation, ignoring"); + break; + default: + printf_debug("\n"); + } + return result; + } /* Send SE (Sector Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared. @@ -623,6 +684,8 @@ { uint32_t pos = 2, size = flash->total_size * 1024; unsigned char w[6] = {0xad, 0, 0, 0, buf[0], buf[1]}; + int result; + switch (flashbus) { case BUS_TYPE_WBSIO_SPI: fprintf(stderr, "%s: impossible with Winbond SPI masters," @@ -632,7 +695,20 @@ break; } flash->erase(flash); - spi_write_enable(); + result = spi_write_enable(); + if (result) { + printf_debug("spi_write_enable failed"); + switch (flashbus) { + case BUS_TYPE_ICH7_SPI: + case BUS_TYPE_ICH9_SPI: + case BUS_TYPE_VIA_SPI: + printf_debug(" due to SPI master limitation, ignoring"); + break; + default: + printf_debug("\n"); + } + return result; + } spi_command(6, 0, w, NULL); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) myusec_delay(5); /* SST25VF040B Tbp is max 10us */ Index: flashrom-spi_write_enable_error_checking/wbsio_spi.c =================================================================== --- flashrom-spi_write_enable_error_checking/wbsio_spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/wbsio_spi.c (Arbeitskopie) @@ -189,6 +189,7 @@ int wbsio_spi_write(struct flashchip *flash, uint8_t *buf) { int pos, size = flash->total_size * 1024; + int result;
if (flash->total_size > 1024) { fprintf(stderr, "%s: Winbond saved on 4 register bits so max chip size is 1024 KB!\n", __func__); @@ -196,7 +197,11 @@ }
flash->erase(flash); - spi_write_enable(); + result = spi_write_enable(); + if (result) { + printf_debug("spi_write_enable failed\n"); + return result; + } for (pos = 0; pos < size; pos++) { spi_byte_program(pos, buf[pos]); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
On 07.05.2009 13:44, Carl-Daniel Hailfinger wrote:
Until the ICH SPI driver can handle preopcodes as standalone opcodes, we should handle such special opcode failure gracefully on ICH and compatible chipsets.
This fixes chip erase on almost all ICH+VIA SPI masters.
Thanks to Ali Nadalizadeh for helping track down this bug!
Improved version, refactored to isolate the impact to a single function.
As a bonus, all invocations of spi_write_enable() now have error checking.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-spi_write_enable_error_checking/it87spi.c =================================================================== --- flashrom-spi_write_enable_error_checking/it87spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/it87spi.c (Arbeitskopie) @@ -196,11 +196,14 @@ }
/* Page size is usually 256 bytes */ -static void it8716f_spi_page_program(int block, uint8_t *buf, uint8_t *bios) +static int it8716f_spi_page_program(int block, uint8_t *buf, uint8_t *bios) { int i; + int result;
- spi_write_enable(); + result = spi_write_enable(); + if (result) + return result; OUTB(0x06, it8716f_flashport + 1); OUTB(((2 + (fast_spi ? 1 : 0)) << 4), it8716f_flashport); for (i = 0; i < 256; i++) { @@ -212,6 +215,7 @@ */ while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) usleep(1000); + return 0; }
/* @@ -222,12 +226,15 @@ { int total_size = 1024 * flash->total_size; int i; + int result;
fast_spi = 0;
spi_disable_blockprotect(); for (i = 0; i < total_size; i++) { - spi_write_enable(); + result = spi_write_enable(); + if (result) + return result; spi_byte_program(i, buf[i]); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) myusec_delay(10); Index: flashrom-spi_write_enable_error_checking/spi.c =================================================================== --- flashrom-spi_write_enable_error_checking/spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/spi.c (Arbeitskopie) @@ -88,9 +88,24 @@ int spi_write_enable(void) { const unsigned char cmd[JEDEC_WREN_OUTSIZE] = { JEDEC_WREN }; + int result;
/* Send WREN (Write Enable) */ - return spi_command(sizeof(cmd), 0, cmd, NULL); + result = spi_command(sizeof(cmd), 0, cmd, NULL); + if (result) { + printf_debug("spi_write_enable failed"); + switch (flashbus) { + case BUS_TYPE_ICH7_SPI: + case BUS_TYPE_ICH9_SPI: + case BUS_TYPE_VIA_SPI: + printf_debug(" due to SPI master limitation, ignoring" + " and hoping it will be run as PREOP\n"); + return 0; + default: + printf_debug("\n"); + } + } + return result; }
int spi_write_disable(void) @@ -361,10 +376,8 @@ return result; } result = spi_write_enable(); - if (result) { - printf_debug("spi_write_enable failed\n"); + if (result) return result; - } /* Send CE (Chip Erase) */ result = spi_command(sizeof(cmd), 0, cmd, NULL); if (result) { @@ -391,10 +404,8 @@ return result; } result = spi_write_enable(); - if (result) { - printf_debug("spi_write_enable failed\n"); + if (result) return result; - } /* Send CE (Chip Erase) */ result = spi_command(sizeof(cmd), 0, cmd, NULL); if (result) { @@ -424,11 +435,14 @@ int spi_block_erase_52(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_BE_52_OUTSIZE] = {JEDEC_BE_52}; + int result;
cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff); - spi_write_enable(); + result = spi_write_enable(); + if (result) + return result; /* Send BE (Block Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared. @@ -447,11 +461,14 @@ int spi_block_erase_d8(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_BE_D8_OUTSIZE] = { JEDEC_BE_D8 }; + int result;
cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff); - spi_write_enable(); + result = spi_write_enable(); + if (result) + return result; /* Send BE (Block Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared. @@ -489,11 +506,15 @@ int spi_sector_erase(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_SE_OUTSIZE] = { JEDEC_SE }; + int result; + cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff);
- spi_write_enable(); + result = spi_write_enable(); + if (result) + return result; /* Send SE (Sector Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared. @@ -623,6 +644,8 @@ { uint32_t pos = 2, size = flash->total_size * 1024; unsigned char w[6] = {0xad, 0, 0, 0, buf[0], buf[1]}; + int result; + switch (flashbus) { case BUS_TYPE_WBSIO_SPI: fprintf(stderr, "%s: impossible with Winbond SPI masters," @@ -632,7 +655,9 @@ break; } flash->erase(flash); - spi_write_enable(); + result = spi_write_enable(); + if (result) + return result; spi_command(6, 0, w, NULL); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) myusec_delay(5); /* SST25VF040B Tbp is max 10us */ Index: flashrom-spi_write_enable_error_checking/wbsio_spi.c =================================================================== --- flashrom-spi_write_enable_error_checking/wbsio_spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/wbsio_spi.c (Arbeitskopie) @@ -189,6 +189,7 @@ int wbsio_spi_write(struct flashchip *flash, uint8_t *buf) { int pos, size = flash->total_size * 1024; + int result;
if (flash->total_size > 1024) { fprintf(stderr, "%s: Winbond saved on 4 register bits so max chip size is 1024 KB!\n", __func__); @@ -196,7 +197,9 @@ }
flash->erase(flash); - spi_write_enable(); + result = spi_write_enable(); + if (result) + return result; for (pos = 0; pos < size; pos++) { spi_byte_program(pos, buf[pos]); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) Index: flashrom-spi_write_enable_error_checking/sb600spi.c =================================================================== --- flashrom-spi_write_enable_error_checking/sb600spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/sb600spi.c (Arbeitskopie) @@ -68,6 +68,7 @@ { int rc = 0, i; int total_size = flash->total_size * 1024; + int result;
/* Erase first */ printf("Erasing flash before programming... "); @@ -77,7 +78,9 @@ printf("Programming flash"); for (i = 0; i < total_size; i++, buf++) { spi_disable_blockprotect(); - spi_write_enable(); + result = spi_write_enable(); + if (result) + return result; spi_byte_program(i, *buf); /* wait program complete. */ if (i % 0x8000 == 0)
Erasing Confirmed working on my hardware : chip-set : "Intel ICH7M" chip : "SST SST25VF080B"
Output image is now completely full of 0xFF (s)
Regards -- Ali Nadalizadeh
On Thu, May 7, 2009 at 4:48 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 07.05.2009 13:44, Carl-Daniel Hailfinger wrote:
Until the ICH SPI driver can handle preopcodes as standalone opcodes, we should handle such special opcode failure gracefully on ICH and compatible chipsets.
This fixes chip erase on almost all ICH+VIA SPI masters.
Thanks to Ali Nadalizadeh for helping track down this bug!
Improved version, refactored to isolate the impact to a single function.
As a bonus, all invocations of spi_write_enable() now have error checking.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-spi_write_enable_error_checking/it87spi.c
--- flashrom-spi_write_enable_error_checking/it87spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/it87spi.c (Arbeitskopie) @@ -196,11 +196,14 @@ }
/* Page size is usually 256 bytes */ -static void it8716f_spi_page_program(int block, uint8_t *buf, uint8_t *bios) +static int it8716f_spi_page_program(int block, uint8_t *buf, uint8_t *bios) { int i;
int result;
spi_write_enable();
result = spi_write_enable();if (result)return result; OUTB(0x06, it8716f_flashport + 1); OUTB(((2 + (fast_spi ? 1 : 0)) << 4), it8716f_flashport); for (i = 0; i < 256; i++) {@@ -212,6 +215,7 @@ */ while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) usleep(1000);
return 0;}
/* @@ -222,12 +226,15 @@ { int total_size = 1024 * flash->total_size; int i;
int result; fast_spi = 0; spi_disable_blockprotect(); for (i = 0; i < total_size; i++) {
spi_write_enable();
result = spi_write_enable();if (result)return result; spi_byte_program(i, buf[i]); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) myusec_delay(10);Index: flashrom-spi_write_enable_error_checking/spi.c
--- flashrom-spi_write_enable_error_checking/spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/spi.c (Arbeitskopie) @@ -88,9 +88,24 @@ int spi_write_enable(void) { const unsigned char cmd[JEDEC_WREN_OUTSIZE] = { JEDEC_WREN };
int result; /* Send WREN (Write Enable) */
return spi_command(sizeof(cmd), 0, cmd, NULL);
result = spi_command(sizeof(cmd), 0, cmd, NULL);if (result) {printf_debug("spi_write_enable failed");switch (flashbus) {case BUS_TYPE_ICH7_SPI:case BUS_TYPE_ICH9_SPI:case BUS_TYPE_VIA_SPI:printf_debug(" due to SPI master limitation,ignoring"
" and hoping it will be run asPREOP\n");
return 0;default:printf_debug("\n");}}return result;}
int spi_write_disable(void) @@ -361,10 +376,8 @@ return result; } result = spi_write_enable();
if (result) {printf_debug("spi_write_enable failed\n");
if (result) return result;
} /* Send CE (Chip Erase) */ result = spi_command(sizeof(cmd), 0, cmd, NULL); if (result) {@@ -391,10 +404,8 @@ return result; } result = spi_write_enable();
if (result) {printf_debug("spi_write_enable failed\n");
if (result) return result;
} /* Send CE (Chip Erase) */ result = spi_command(sizeof(cmd), 0, cmd, NULL); if (result) {@@ -424,11 +435,14 @@ int spi_block_erase_52(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_BE_52_OUTSIZE] = {JEDEC_BE_52};
int result; cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff);
spi_write_enable();
result = spi_write_enable();if (result)return result; /* Send BE (Block Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared.@@ -447,11 +461,14 @@ int spi_block_erase_d8(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_BE_D8_OUTSIZE] = { JEDEC_BE_D8 };
int result; cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff);
spi_write_enable();
result = spi_write_enable();if (result)return result; /* Send BE (Block Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared.@@ -489,11 +506,15 @@ int spi_sector_erase(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_SE_OUTSIZE] = { JEDEC_SE };
int result;cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff);
spi_write_enable();
result = spi_write_enable();if (result)return result; /* Send SE (Sector Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared.@@ -623,6 +644,8 @@ { uint32_t pos = 2, size = flash->total_size * 1024; unsigned char w[6] = {0xad, 0, 0, 0, buf[0], buf[1]};
int result;switch (flashbus) { case BUS_TYPE_WBSIO_SPI: fprintf(stderr, "%s: impossible with Winbond SPI masters,"@@ -632,7 +655,9 @@ break; } flash->erase(flash);
spi_write_enable();
result = spi_write_enable();if (result)return result; spi_command(6, 0, w, NULL); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) myusec_delay(5); /* SST25VF040B Tbp is max 10us */Index: flashrom-spi_write_enable_error_checking/wbsio_spi.c
--- flashrom-spi_write_enable_error_checking/wbsio_spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/wbsio_spi.c (Arbeitskopie) @@ -189,6 +189,7 @@ int wbsio_spi_write(struct flashchip *flash, uint8_t *buf) { int pos, size = flash->total_size * 1024;
int result; if (flash->total_size > 1024) { fprintf(stderr, "%s: Winbond saved on 4 register bits so maxchip size is 1024 KB!\n", __func__); @@ -196,7 +197,9 @@ }
flash->erase(flash);
spi_write_enable();
result = spi_write_enable();if (result)return result; for (pos = 0; pos < size; pos++) { spi_byte_program(pos, buf[pos]); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)Index: flashrom-spi_write_enable_error_checking/sb600spi.c
--- flashrom-spi_write_enable_error_checking/sb600spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/sb600spi.c (Arbeitskopie) @@ -68,6 +68,7 @@ { int rc = 0, i; int total_size = flash->total_size * 1024;
int result; /* Erase first */ printf("Erasing flash before programming... ");@@ -77,7 +78,9 @@ printf("Programming flash"); for (i = 0; i < total_size; i++, buf++) { spi_disable_blockprotect();
spi_write_enable();
result = spi_write_enable();if (result)return result; spi_byte_program(i, *buf); /* wait program complete. */ if (i % 0x8000 == 0)Index: flashrom-spi_write_enable_error_checking/it87spi.c
--- flashrom-spi_write_enable_error_checking/it87spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/it87spi.c (Arbeitskopie) @@ -196,11 +196,14 @@ }
/* Page size is usually 256 bytes */ -static void it8716f_spi_page_program(int block, uint8_t *buf, uint8_t *bios) +static int it8716f_spi_page_program(int block, uint8_t *buf, uint8_t *bios) { int i;
int result;
spi_write_enable();
result = spi_write_enable();if (result)return result; OUTB(0x06, it8716f_flashport + 1); OUTB(((2 + (fast_spi ? 1 : 0)) << 4), it8716f_flashport); for (i = 0; i < 256; i++) {@@ -212,6 +215,7 @@ */ while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) usleep(1000);
return 0;}
/* @@ -222,12 +226,15 @@ { int total_size = 1024 * flash->total_size; int i;
int result; fast_spi = 0; spi_disable_blockprotect(); for (i = 0; i < total_size; i++) {
spi_write_enable();
result = spi_write_enable();if (result)return result; spi_byte_program(i, buf[i]); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) myusec_delay(10);Index: flashrom-spi_write_enable_error_checking/spi.c
--- flashrom-spi_write_enable_error_checking/spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/spi.c (Arbeitskopie) @@ -88,9 +88,24 @@ int spi_write_enable(void) { const unsigned char cmd[JEDEC_WREN_OUTSIZE] = { JEDEC_WREN };
int result; /* Send WREN (Write Enable) */
return spi_command(sizeof(cmd), 0, cmd, NULL);
result = spi_command(sizeof(cmd), 0, cmd, NULL);if (result) {printf_debug("spi_write_enable failed");switch (flashbus) {case BUS_TYPE_ICH7_SPI:case BUS_TYPE_ICH9_SPI:case BUS_TYPE_VIA_SPI:printf_debug(" due to SPI master limitation,ignoring"
" and hoping it will be run asPREOP\n");
return 0;default:printf_debug("\n");}}return result;}
int spi_write_disable(void) @@ -361,10 +376,8 @@ return result; } result = spi_write_enable();
if (result) {printf_debug("spi_write_enable failed\n");
if (result) return result;
} /* Send CE (Chip Erase) */ result = spi_command(sizeof(cmd), 0, cmd, NULL); if (result) {@@ -391,10 +404,8 @@ return result; } result = spi_write_enable();
if (result) {printf_debug("spi_write_enable failed\n");
if (result) return result;
} /* Send CE (Chip Erase) */ result = spi_command(sizeof(cmd), 0, cmd, NULL); if (result) {@@ -424,11 +435,14 @@ int spi_block_erase_52(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_BE_52_OUTSIZE] = {JEDEC_BE_52};
int result; cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff);
spi_write_enable();
result = spi_write_enable();if (result)return result; /* Send BE (Block Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared.@@ -447,11 +461,14 @@ int spi_block_erase_d8(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_BE_D8_OUTSIZE] = { JEDEC_BE_D8 };
int result; cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff);
spi_write_enable();
result = spi_write_enable();if (result)return result; /* Send BE (Block Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared.@@ -489,11 +506,15 @@ int spi_sector_erase(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_SE_OUTSIZE] = { JEDEC_SE };
int result;cmd[1] = (addr & 0x00ff0000) >> 16; cmd[2] = (addr & 0x0000ff00) >> 8; cmd[3] = (addr & 0x000000ff);
spi_write_enable();
result = spi_write_enable();if (result)return result; /* Send SE (Sector Erase) */ spi_command(sizeof(cmd), 0, cmd, NULL); /* Wait until the Write-In-Progress bit is cleared.@@ -623,6 +644,8 @@ { uint32_t pos = 2, size = flash->total_size * 1024; unsigned char w[6] = {0xad, 0, 0, 0, buf[0], buf[1]};
int result;switch (flashbus) { case BUS_TYPE_WBSIO_SPI: fprintf(stderr, "%s: impossible with Winbond SPI masters,"@@ -632,7 +655,9 @@ break; } flash->erase(flash);
spi_write_enable();
result = spi_write_enable();if (result)return result; spi_command(6, 0, w, NULL); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) myusec_delay(5); /* SST25VF040B Tbp is max 10us */Index: flashrom-spi_write_enable_error_checking/wbsio_spi.c
--- flashrom-spi_write_enable_error_checking/wbsio_spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/wbsio_spi.c (Arbeitskopie) @@ -189,6 +189,7 @@ int wbsio_spi_write(struct flashchip *flash, uint8_t *buf) { int pos, size = flash->total_size * 1024;
int result; if (flash->total_size > 1024) { fprintf(stderr, "%s: Winbond saved on 4 register bits so maxchip size is 1024 KB!\n", __func__); @@ -196,7 +197,9 @@ }
flash->erase(flash);
spi_write_enable();
result = spi_write_enable();if (result)return result; for (pos = 0; pos < size; pos++) { spi_byte_program(pos, buf[pos]); while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)Index: flashrom-spi_write_enable_error_checking/sb600spi.c
--- flashrom-spi_write_enable_error_checking/sb600spi.c (Revision 471) +++ flashrom-spi_write_enable_error_checking/sb600spi.c (Arbeitskopie) @@ -68,6 +68,7 @@ { int rc = 0, i; int total_size = flash->total_size * 1024;
int result; /* Erase first */ printf("Erasing flash before programming... ");@@ -77,7 +78,9 @@ printf("Programming flash"); for (i = 0; i < total_size; i++, buf++) { spi_disable_blockprotect();
spi_write_enable();
result = spi_write_enable();if (result)return result; spi_byte_program(i, *buf); /* wait program complete. */ if (i % 0x8000 == 0)-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
On 07.05.2009 14:18, Carl-Daniel Hailfinger wrote:
On 07.05.2009 13:44, Carl-Daniel Hailfinger wrote:
Until the ICH SPI driver can handle preopcodes as standalone opcodes, we should handle such special opcode failure gracefully on ICH and compatible chipsets.
This fixes chip erase on almost all ICH+VIA SPI masters.
Thanks to Ali Nadalizadeh for helping track down this bug!
Improved version, refactored to isolate the impact to a single function.
As a bonus, all invocations of spi_write_enable() now have error checking.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Self-acked and committed in r484. I could not leave a killer bug in flashrom any longer.
Regards, Carl-Daniel
Carl-Daniel wrote:
Until the ICH SPI driver can handle preopcodes as standalone opcodes ...
I don't like ICH's preop design either. However, I think we shall write software following the hardware design's policy. Trying to compensate defect of hardware design costs and probably not gains more.
ICH SPI tries to be "smarter" than other chipsets. We may consider transfer control to ICH SPI driver as early as possible, leaving more decision to be done by the ICH SPI driver instead of the generic SPI driver layer.
My two cents.
yu ning
On 07.05.2009 18:25, FENG Yu Ning wrote:
Carl-Daniel wrote:
Until the ICH SPI driver can handle preopcodes as standalone opcodes ...
I don't like ICH's preop design either. However, I think we shall write software following the hardware design's policy. Trying to compensate defect of hardware design costs and probably not gains more.
I beg to differ. Since we want to keep as much code as possible shared for every flasher (SPI master), working around design bugs is more efficient than changing flashrom to conform to every weird piece of hardware out there.
ICH SPI tries to be "smarter" than other chipsets. We may consider transfer control to ICH SPI driver as early as possible, leaving more decision to be done by the ICH SPI driver instead of the generic SPI driver layer.
Right now, drivers can override many SPI generic operations as needed. If the abstraction is now powerful enough, I'm all for improving it. Adding support for external programmers will certainly entail new challenges.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
I beg to differ. Since we want to keep as much code as possible shared for every flasher (SPI master), working around design bugs is more efficient than changing flashrom to conform to every weird piece of hardware out there.
Sometimes it is just difficult, but I see your point.
Right now, drivers can override many SPI generic operations as needed. If the abstraction is now powerful enough, I'm all for improving it.
The abstraction is ok then. I just have some words on how generic SPI layer sends commands to specific drivers. Now it does that by specifying byte sequences that are transfered on the wire. Some semantic information is lost here and the driver code has to guess. The ICH SPI driver code looks really bad in the design(I have contributed some to that) and coding(no offensive to anyone). I think command specification has something to do with the bad design part.
Sorry for looking from the ICH SPI's view so often, but I just seriously read that part so care more about it than other parts.
Adding support for external programmers will certainly entail new challenges.
Challenges bring opportunity to improve.
yu ning
-
Ali Nadalizadeh -
Carl-Daniel Hailfinger -
FENG Yu Ning -
Peter Stuge -
Rudolf Marek