Check for failed SPI command execution in flashrom. Although SPI itself does not have a mechanism to signal command failure, the SPI host may be unable to send a given command over the wire due to security or hardware limitations. The current code ignores these mechanisms completely and simply assumes almost every command succeeds. Complain if SPI command execution fails.
Since locked down Intel chipsets (like the one we had problems with earlier) only allow a small subset of commands, find the common subset of commands between the chipset and the ROM in the chip erase case. That is accomplished by the new spi_chip_erase_60_c7() which can be used for chips supporting both 0x60 and 0xc7 chip erase commands.
Both parts of the patch address problems seen in the real world. The increased verbosity for the error case will help us diagnose and address problems better.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-spi_error_checking/flash.h =================================================================== --- flashrom-spi_error_checking/flash.h (Revision 3754) +++ flashrom-spi_error_checking/flash.h (Arbeitskopie) @@ -452,19 +452,20 @@ int probe_spi_res(struct flashchip *flash); int spi_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); -void spi_write_enable(); -void spi_write_disable(); +int spi_write_enable(); +int spi_write_disable(); int spi_chip_erase_60(struct flashchip *flash); int spi_chip_erase_c7(struct flashchip *flash); +int spi_chip_erase_60_c7(struct flashchip *flash); int spi_chip_erase_d8(struct flashchip *flash); int spi_block_erase_52(const struct flashchip *flash, unsigned long addr); int spi_block_erase_d8(const struct flashchip *flash, unsigned long addr); int spi_chip_write(struct flashchip *flash, uint8_t *buf); int spi_chip_read(struct flashchip *flash, uint8_t *buf); uint8_t spi_read_status_register(); -void spi_disable_blockprotect(void); +int spi_disable_blockprotect(void); void spi_byte_program(int address, uint8_t byte); -void spi_nbyte_read(int address, uint8_t *bytes, int len); +int spi_nbyte_read(int address, uint8_t *bytes, int len);
/* 82802ab.c */ int probe_82802ab(struct flashchip *flash); Index: flashrom-spi_error_checking/spi.c =================================================================== --- flashrom-spi_error_checking/spi.c (Revision 3754) +++ flashrom-spi_error_checking/spi.c (Arbeitskopie) @@ -71,20 +71,20 @@ return 0; }
-void spi_write_enable() +int spi_write_enable() { const unsigned char cmd[JEDEC_WREN_OUTSIZE] = { JEDEC_WREN };
/* Send WREN (Write Enable) */ - spi_command(sizeof(cmd), 0, cmd, NULL); + return spi_command(sizeof(cmd), 0, cmd, NULL); }
-void spi_write_disable() +int spi_write_disable() { const unsigned char cmd[JEDEC_WRDI_OUTSIZE] = { JEDEC_WRDI };
/* Send WRDI (Write Disable) */ - spi_command(sizeof(cmd), 0, cmd, NULL); + return spi_command(sizeof(cmd), 0, cmd, NULL); }
static int probe_spi_rdid_generic(struct flashchip *flash, int bytes) @@ -274,14 +274,28 @@ int spi_chip_erase_60(struct flashchip *flash) { const unsigned char cmd[JEDEC_CE_60_OUTSIZE] = {JEDEC_CE_60}; + int result; - spi_disable_blockprotect(); - spi_write_enable(); + result = spi_disable_blockprotect(); + if (result) { + printf_debug("spi_disable_blockprotect failed\n"); + return result; + } + result = spi_write_enable(); + if (result) { + printf_debug("spi_write_enable failed\n"); + return result; + } /* Send CE (Chip Erase) */ - spi_command(sizeof(cmd), 0, cmd, NULL); + result = spi_command(sizeof(cmd), 0, cmd, NULL); + if (result) { + printf_debug("spi_chip_erase_60 failed sending erase\n"); + return result; + } /* Wait until the Write-In-Progress bit is cleared. * This usually takes 1-85 s, so wait in 1 s steps. */ + /* FIXME: We assume spi_read_status_register will never fail. */ while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) sleep(1); return 0; @@ -290,19 +304,44 @@ int spi_chip_erase_c7(struct flashchip *flash) { const unsigned char cmd[JEDEC_CE_C7_OUTSIZE] = { JEDEC_CE_C7 }; + int result;
- spi_disable_blockprotect(); - spi_write_enable(); + result = spi_disable_blockprotect(); + if (result) { + printf_debug("spi_disable_blockprotect failed\n"); + return result; + } + result = spi_write_enable(); + if (result) { + printf_debug("spi_write_enable failed\n"); + return result; + } /* Send CE (Chip Erase) */ - spi_command(sizeof(cmd), 0, cmd, NULL); + result = spi_command(sizeof(cmd), 0, cmd, NULL); + if (result) { + printf_debug("spi_chip_erase_60 failed sending erase\n"); + return result; + } /* Wait until the Write-In-Progress bit is cleared. * This usually takes 1-85 s, so wait in 1 s steps. */ + /* FIXME: We assume spi_read_status_register will never fail. */ while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) sleep(1); return 0; }
+int spi_chip_erase_60_c7(struct flashchip *flash) +{ + int result; + result = spi_chip_erase_60(flash); + if (result) { + printf_debug("spi_chip_erase_60 failed, trying c7\n"); + result = spi_chip_erase_c7(flash); + } + return result; +} + int spi_block_erase_52(const struct flashchip *flash, unsigned long addr) { unsigned char cmd[JEDEC_BE_52_OUTSIZE] = {JEDEC_BE_52}; @@ -390,13 +429,13 @@ * This is according the SST25VF016 datasheet, who knows it is more * generic that this... */ -void spi_write_status_register(int status) +int spi_write_status_register(int status) { const unsigned char cmd[JEDEC_WRSR_OUTSIZE] = { JEDEC_WRSR, (unsigned char)status };
/* Send WRSR (Write Status Register) */ - spi_command(sizeof(cmd), 0, cmd, NULL); + return spi_command(sizeof(cmd), 0, cmd, NULL); }
void spi_byte_program(int address, uint8_t byte) @@ -413,20 +452,30 @@ spi_command(sizeof(cmd), 0, cmd, NULL); }
-void spi_disable_blockprotect(void) +int spi_disable_blockprotect(void) { uint8_t status; + int result;
status = spi_read_status_register(); /* If there is block protection in effect, unprotect it first. */ if ((status & 0x3c) != 0) { printf_debug("Some block protection in effect, disabling\n"); - spi_write_enable(); - spi_write_status_register(status & ~0x3c); + result = spi_write_enable(); + if (result) { + printf_debug("spi_write_enable failed\n"); + return result; + } + result = spi_write_status_register(status & ~0x3c); + if (result) { + printf_debug("spi_write_status_register failed\n"); + return result; + } } + return 0; }
-void spi_nbyte_read(int address, uint8_t *bytes, int len) +int spi_nbyte_read(int address, uint8_t *bytes, int len) { const unsigned char cmd[JEDEC_READ_OUTSIZE] = { JEDEC_READ, @@ -436,7 +485,7 @@ };
/* Send Read */ - spi_command(sizeof(cmd), len, cmd, bytes); + return spi_command(sizeof(cmd), len, cmd, bytes); }
int spi_chip_read(struct flashchip *flash, uint8_t *buf)
Carl-Daniel Hailfinger wrote:
Check for failed SPI command execution in flashrom. Although SPI itself does not have a mechanism to signal command failure, the SPI host may be unable to send a given command over the wire due to security or hardware limitations. The current code ignores these mechanisms completely and simply assumes almost every command succeeds. Complain if SPI command execution fails.
Since locked down Intel chipsets (like the one we had problems with earlier) only allow a small subset of commands, find the common subset of commands between the chipset and the ROM in the chip erase case. That is accomplished by the new spi_chip_erase_60_c7() which can be used for chips supporting both 0x60 and 0xc7 chip erase commands.
Both parts of the patch address problems seen in the real world. The increased verbosity for the error case will help us diagnose and address problems better.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) sleep(1); return 0; @@ -290,19 +304,44 @@ int spi_chip_erase_c7(struct flashchip *flash) { const unsigned char cmd[JEDEC_CE_C7_OUTSIZE] = { JEDEC_CE_C7 };
- int result;
- spi_disable_blockprotect();
- spi_write_enable();
- result = spi_disable_blockprotect();
- if (result) {
printf_debug("spi_disable_blockprotect failed\n");return result;- }
- result = spi_write_enable();
- if (result) {
printf_debug("spi_write_enable failed\n");return result;- }
This code is duplicated in a couple of places. Is it worth factoring it out?
+int spi_chip_erase_60_c7(struct flashchip *flash) +{
- int result;
- result = spi_chip_erase_60(flash);
- if (result) {
printf_debug("spi_chip_erase_60 failed, trying c7\n");result = spi_chip_erase_c7(flash);- }
- return result;
+}
I don't particularly like this. Maybe we should have spi_chip_erase() try all different erase functions in a row, and check whether the erase was actually successfull (all bytes 0xff)?
Otherwise: Acked-by: Stefan Reinauer stepan@coresystems.de
On 17.11.2008 14:28, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
Check for failed SPI command execution in flashrom. Although SPI itself does not have a mechanism to signal command failure, the SPI host may be unable to send a given command over the wire due to security or hardware limitations. The current code ignores these mechanisms completely and simply assumes almost every command succeeds. Complain if SPI command execution fails.
Since locked down Intel chipsets (like the one we had problems with earlier) only allow a small subset of commands, find the common subset of commands between the chipset and the ROM in the chip erase case. That is accomplished by the new spi_chip_erase_60_c7() which can be used for chips supporting both 0x60 and 0xc7 chip erase commands.
Both parts of the patch address problems seen in the real world. The increased verbosity for the error case will help us diagnose and address problems better.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) sleep(1); return 0; @@ -290,19 +304,44 @@ int spi_chip_erase_c7(struct flashchip *flash) { const unsigned char cmd[JEDEC_CE_C7_OUTSIZE] = { JEDEC_CE_C7 };
- int result;
- spi_disable_blockprotect();
- spi_write_enable();
- result = spi_disable_blockprotect();
- if (result) {
printf_debug("spi_disable_blockprotect failed\n");return result;- }
- result = spi_write_enable();
- if (result) {
printf_debug("spi_write_enable failed\n");return result;- }
This code is duplicated in a couple of places. Is it worth factoring it out?
I tried factoring it out, but since we don't want to die() completely, we have to return with an error code from this function. That pretty much rules out factoring out the checks into a separate function. I could move all checks one level down in the call graph and clutter up only the end of each function. What do you think?
+int spi_chip_erase_60_c7(struct flashchip *flash) +{
- int result;
- result = spi_chip_erase_60(flash);
- if (result) {
printf_debug("spi_chip_erase_60 failed, trying c7\n");result = spi_chip_erase_c7(flash);- }
- return result;
+}
I don't particularly like this. Maybe we should have spi_chip_erase() try all different erase functions in a row, and check whether the erase was actually successfull (all bytes 0xff)?
That's a bit difficult. There are some chips which have either the 0x60 or the 0xc7 opcode used for something else, so we need to keep this chip specific.
Your point about the check for successful erase is definitely valid and I'll post a followup patch to check this.
Otherwise: Acked-by: Stefan Reinauer stepan@coresystems.de
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
On 17.11.2008 14:28, Stefan Reinauer wrote:
I tried factoring it out, but since we don't want to die() completely, we have to return with an error code from this function. That pretty much rules out factoring out the checks into a separate function. I could move all checks one level down in the call graph and clutter up only the end of each function. What do you think?
I guess your current version is fine then
+int spi_chip_erase_60_c7(struct flashchip *flash) +{
- int result;
- result = spi_chip_erase_60(flash);
- if (result) {
printf_debug("spi_chip_erase_60 failed, trying c7\n");result = spi_chip_erase_c7(flash);- }
- return result;
+}
I don't particularly like this. Maybe we should have spi_chip_erase() try all different erase functions in a row, and check whether the erase was actually successfull (all bytes 0xff)?
That's a bit difficult. There are some chips which have either the 0x60 or the 0xc7 opcode used for something else, so we need to keep this chip specific.
Hm.. So do we need to keep an array of supported read, write, erase, id opcodes for each spi chip to handle this? That way we could check the chip's capabilities and compare to the OPMENUs capabilities.
That one function does one job, but I'm a little concerned that we end up writing functions for all possible variations of possible commands and have a hard time tracking it afterwards.
Stefan
On 17.11.2008 15:04, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 17.11.2008 14:28, Stefan Reinauer wrote:
I tried factoring it out, but since we don't want to die() completely, we have to return with an error code from this function. That pretty much rules out factoring out the checks into a separate function. I could move all checks one level down in the call graph and clutter up only the end of each function. What do you think?
I guess your current version is fine then
OK, thanks.
+int spi_chip_erase_60_c7(struct flashchip *flash) +{
- int result;
- result = spi_chip_erase_60(flash);
- if (result) {
printf_debug("spi_chip_erase_60 failed, trying c7\n");result = spi_chip_erase_c7(flash);- }
- return result;
+}
I don't particularly like this. Maybe we should have spi_chip_erase() try all different erase functions in a row, and check whether the erase was actually successfull (all bytes 0xff)?
That's a bit difficult. There are some chips which have either the 0x60 or the 0xc7 opcode used for something else, so we need to keep this chip specific.
Hm.. So do we need to keep an array of supported read, write, erase, id opcodes for each spi chip to handle this? That way we could check the chip's capabilities and compare to the OPMENUs capabilities.
For read at normal speed, it's the same opcode for all SPI EEPROM chips with power-of-two sizes I saw so far. For id, there are up to four opcodes, each of them yielding a different answer.
That one function does one job, but I'm a little concerned that we end up writing functions for all possible variations of possible commands and have a hard time tracking it afterwards.
The most sane way for SPI would be to bundle valid opcodes for a given function together with the function and any auxilliary data for that function. Let me give an example:
struct available_command { u32 opcodes[4]; int somefunction(int param1, int param2, int param3, void* param4, void* param5); char auxiliary_data[64]; }
struct available_command chip_erase = { .opcodes = {0x60, 0xc7,} .somefunction = chip_erase(0,0,0, struct flashchip, chip_erase.opcodes, NULL); .auxiliary_data = {}; }
For sector erase, auxiliary_data would hold sector sizes.
Regards, Carl-Daniel
Stefan Reinauer wrote:
Hm.. So do we need to keep an array of supported read, write, erase, id opcodes for each spi chip to handle this?
Yes, that is the best we can do. We should also keep track of which commands can be executed on which SPI masters; some have limited command sets, others only have limits on number of bytes transferred in command and response, which in turn limits which commands can be sent.
That one function does one job, but I'm a little concerned that we end up writing functions for all possible variations of possible commands and have a hard time tracking it afterwards.
Until flashrom can understand the entire path to the chip I think it's fine.
//Peter
On 17.11.2008 14:28, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
Check for failed SPI command execution in flashrom. Although SPI itself does not have a mechanism to signal command failure, the SPI host may be unable to send a given command over the wire due to security or hardware limitations. The current code ignores these mechanisms completely and simply assumes almost every command succeeds. Complain if SPI command execution fails.
Since locked down Intel chipsets (like the one we had problems with earlier) only allow a small subset of commands, find the common subset of commands between the chipset and the ROM in the chip erase case. That is accomplished by the new spi_chip_erase_60_c7() which can be used for chips supporting both 0x60 and 0xc7 chip erase commands.
Both parts of the patch address problems seen in the real world. The increased verbosity for the error case will help us diagnose and address problems better.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Stefan Reinauer stepan@coresystems.de
Thanks, committed in r3757.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Peter Stuge -
Stefan Reinauer