Hello Hung-Te Lin, build bot (Jenkins), Paul Menzel, Stefan Reinauer, David Hendricks, Edward O'Callaghan, Angel Pons, Samir Ibradžić,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/flashrom/+/40626
to review the following change.
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Revert "flashchips: port S25FS(128S) chip from chromiumos"
This reverts commit a3519561bd0fb44153bb376322b799000657576f.
Breaks support for most SPI flash chips. Doesn't look like it's been reviewed at all. It's too big and too invasive to be reviewed as a single commit.
Change-Id: Id2a4593bdb654f8a26957d69d52189ce61621d93 --- M chipdrivers.h M flash.h M flashchips.c M flashchips.h M flashrom.c M spi.h M spi25.c M spi25_statusreg.c 8 files changed, 6 insertions(+), 415 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/26/40626/1
diff --git a/chipdrivers.h b/chipdrivers.h index aa24085..cb1e877 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -35,9 +35,6 @@ int probe_spi_res2(struct flashctx *flash); int probe_spi_res3(struct flashctx *flash); int probe_spi_at25f(struct flashctx *flash); -int probe_spi_big_spansion(struct flashctx *flash); -int s25fs_software_reset(struct flashctx *flash); -int spi_poll_wip(struct flashctx *const flash, const unsigned int poll_delay); int spi_write_enable(struct flashctx *flash); int spi_write_disable(struct flashctx *flash); int spi_block_erase_20(struct flashctx *flash, unsigned int addr, unsigned int blocklen); @@ -52,7 +49,6 @@ int spi_block_erase_c7(struct flashctx *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_d7(struct flashctx *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_d8(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int s25fs_block_erase_d8(struct flashctx *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_db(struct flashctx *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_dc(struct flashctx *flash, unsigned int addr, unsigned int blocklen); erasefunc_t *spi_get_erasefn_from_opcode(uint8_t opcode); @@ -64,12 +60,10 @@ int spi_exit_4ba(struct flashctx *flash); int spi_set_extended_address(struct flashctx *, uint8_t addr_high);
+ /* spi25_statusreg.c */ uint8_t spi_read_status_register(struct flashctx *flash); int spi_write_status_register(struct flashctx *flash, int status); -int s25fs_read_cr(struct flashctx *const flash, uint32_t addr); -int s25fs_write_cr(struct flashctx *const flash, uint32_t addr, uint8_t data); -int s25fs_restore_cr3nv(struct flashctx *const flash, uint8_t cfg); void spi_prettyprint_status_register_bit(uint8_t status, int bit); int spi_prettyprint_status_register_plain(struct flashctx *flash); int spi_prettyprint_status_register_default_welwip(struct flashctx *flash); diff --git a/flash.h b/flash.h index 1ee0f0f..1a9bd9f 100644 --- a/flash.h +++ b/flash.h @@ -54,8 +54,6 @@ #define PRIxPTR_WIDTH ((int)(sizeof(uintptr_t)*2))
int register_shutdown(int (*function) (void *data), void *data); -#define CHIP_RESTORE_CALLBACK int (*func) (struct flashrom_flashctx *flash, uint8_t status) -int register_chip_restore(CHIP_RESTORE_CALLBACK, struct flashrom_flashctx *flash, uint8_t status); int shutdown_free(void *data); void *programmer_map_flash_region(const char *descr, uintptr_t phys_addr, size_t len); void programmer_unmap_flash_region(void *virt_addr, size_t len); @@ -209,7 +207,6 @@ SPI_EDI = 1, } spi_cmd_set;
- int (*reset) (struct flashctx *flash); int (*probe) (struct flashctx *flash);
/* Delay after "enter/exit ID mode" commands in microseconds. diff --git a/flashchips.c b/flashchips.c index 974ba81..b4006df 100644 --- a/flashchips.c +++ b/flashchips.c @@ -49,7 +49,6 @@ * .eraseblocks[] = Array of { blocksize, blockcount } * .block_erase = Block erase function * } - * .reset = Reset Chip * .printlock = Chip lock status function * .unlock = Chip unlock function * .write = Chip write function @@ -15826,69 +15825,6 @@
{ .vendor = "Spansion", - .name = "S25FS128S Large Sectors", - .bustype = BUS_SPI, - .manufacture_id = SPANSION_ID, - .model_id = SPANSION_S25FS128S_L, - .total_size = 16384, - .page_size = 256, - .feature_bits = FEATURE_WRSR_WREN, - .tested = TEST_UNTESTED, - .probe = probe_spi_big_spansion, - .probe_timing = TIMING_ZERO, - .block_erasers = - { - { - .eraseblocks = { {64 * 1024, 256} }, - .block_erase = s25fs_block_erase_d8, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_60, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_c7, - }, - }, - .unlock = spi_disable_blockprotect, - .write = spi_chip_write_256, - .read = spi_chip_read, - .voltage = {1700, 2000}, - }, - - { - .vendor = "Spansion", - .name = "S25FS128S Small Sectors", - .bustype = BUS_SPI, - .manufacture_id = SPANSION_ID, - .model_id = SPANSION_S25FS128S_S, - .total_size = 16384, - .page_size = 256, - .feature_bits = FEATURE_WRSR_WREN, - .tested = TEST_OK_PREW, - .probe = probe_spi_big_spansion, - .probe_timing = TIMING_ZERO, - .block_erasers = - { - { - .eraseblocks = { {64 * 1024, 256} }, - .block_erase = s25fs_block_erase_d8, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_60, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_c7, - }, - }, - .reset = s25fs_software_reset, - .unlock = spi_disable_blockprotect, - .write = spi_chip_write_256, - .read = spi_chip_read, - .voltage = {1700, 2000}, - }, - - { - .vendor = "Spansion", .name = "S25FL129P......1", /* uniform 256 kB sectors */ .bustype = BUS_SPI, .manufacture_id = SPANSION_ID, diff --git a/flashchips.h b/flashchips.h index 0c77d1d..e5ef390 100644 --- a/flashchips.h +++ b/flashchips.h @@ -642,8 +642,6 @@ #define SPANSION_S25FL032A 0x0215 /* Same as S25FL032P, but the latter supports EDI and CFI */ #define SPANSION_S25FL064A 0x0216 /* Same as S25FL064P, but the latter supports EDI and CFI */ #define SPANSION_S25FL128 0x2018 /* Same ID for various S25FL127S, S25FL128P, S25FL128S and S25FL129P (including dual-die S70FL256P) variants (EDI supported) */ -#define SPANSION_S25FS128S_L 0x20180081 /* Large sectors. */ -#define SPANSION_S25FS128S_S 0x20180181 /* Small sectors. */ #define SPANSION_S25FL256 0x0219 #define SPANSION_S25FL512 0x0220 #define SPANSION_S25FL204 0x4013 diff --git a/flashrom.c b/flashrom.c index 4224637..07ce734 100644 --- a/flashrom.c +++ b/flashrom.c @@ -500,14 +500,6 @@ {0}, /* This entry corresponds to PROGRAMMER_INVALID. */ };
-#define CHIP_RESTORE_MAXFN 4 -static int chip_restore_fn_count = 0; -static struct chip_restore_func_data { - CHIP_RESTORE_CALLBACK; - struct flashctx *flash; - uint8_t status; -} chip_restore_fn[CHIP_RESTORE_MAXFN]; - #define SHUTDOWN_MAXFN 32 static int shutdown_fn_count = 0; /** @private */ @@ -558,23 +550,6 @@ return 0; }
-//int register_chip_restore(int (*function) (void *data), void *data) -int register_chip_restore(CHIP_RESTORE_CALLBACK, - struct flashctx *flash, uint8_t status) -{ - if (chip_restore_fn_count >= CHIP_RESTORE_MAXFN) { - msg_perr("Tried to register more than %i chip restore" - " functions.\n", CHIP_RESTORE_MAXFN); - return 1; - } - chip_restore_fn[chip_restore_fn_count].func = func; /* from macro */ - chip_restore_fn[chip_restore_fn_count].flash = flash; - chip_restore_fn[chip_restore_fn_count].status = status; - chip_restore_fn_count++; - - return 0; -} - int programmer_init(enum programmer prog, const char *param) { int ret; diff --git a/spi.h b/spi.h index 12063d7..3f45038 100644 --- a/spi.h +++ b/spi.h @@ -101,7 +101,7 @@ #define JEDEC_BE_C4_OUTSIZE 0x04 #define JEDEC_BE_C4_INSIZE 0x00
-/* Block Erase 0xd8 is supported by EON/Macronix/Spansion chips. */ +/* Block Erase 0xd8 is supported by EON/Macronix chips. */ #define JEDEC_BE_D8 0xd8 #define JEDEC_BE_D8_OUTSIZE 0x04 #define JEDEC_BE_D8_INSIZE 0x00 @@ -116,18 +116,6 @@ #define JEDEC_SE_OUTSIZE 0x04 #define JEDEC_SE_INSIZE 0x00
-/* RADR, WRAR, RSTEN, RST & CR3NV OPs and timers on Spansion S25FS chips */ -#define CMD_RDAR 0x65 -#define CMD_WRAR 0x71 -#define CMD_WRAR_LEN 5 -#define CMD_RSTEN 0x66 -#define CMD_RST 0x99 -#define CR3NV_ADDR 0x000004 -#define CR3NV_20H_NV (1 << 3) -#define T_W 145 * 1000 /* NV register write time */ -#define T_RPH 35 /* Reset pulse hold time */ -#define T_SE 145 * 1000 /* Sector Erase Time */ - /* Page Erase 0xDB */ #define JEDEC_PE 0xDB #define JEDEC_PE_OUTSIZE 0x04 @@ -141,7 +129,6 @@ /* Status Register Bits */ #define SPI_SR_WIP (0x01 << 0) #define SPI_SR_WEL (0x01 << 1) -#define SPI_SR_ERA_ERR (0x01 << 5) #define SPI_SR_AAI (0x01 << 6)
/* Write Status Enable */ diff --git a/spi25.c b/spi25.c index c774f99..2a1d492 100644 --- a/spi25.c +++ b/spi25.c @@ -25,7 +25,6 @@ #include "flashchips.h" #include "chipdrivers.h" #include "programmer.h" -#include "hwaccess.h" #include "spi.h"
static int spi_rdid(struct flashctx *flash, unsigned char *readarr, int bytes) @@ -285,155 +284,13 @@ return 0; }
-/* Used for probing 'big' Spansion/Cypress S25FS chips */ -int probe_spi_big_spansion(struct flashctx *flash) +static int spi_poll_wip(struct flashctx *const flash, const unsigned int poll_delay) { - static const unsigned char cmd = JEDEC_RDID; - int ret; - unsigned char dev_id[6]; /* We care only about 6 first bytes */ - - ret = spi_send_command(flash, sizeof(cmd), sizeof(dev_id), &cmd, dev_id); - - if (!ret) { - unsigned long i; - - for (i = 0; i < sizeof(dev_id); i++) - msg_gdbg(" 0x%02x", dev_id[i]); - msg_gdbg(".\n"); - - if (dev_id[0] == flash->chip->manufacture_id) { - union { - uint8_t array[4]; - uint32_t whole; - } model_id; - - /* - * The structure of the RDID output is as follows: - * - * offset value meaning - * 00h 01h Manufacturer ID for Spansion - * 01h 20h 128 Mb capacity - * 01h 02h 256 Mb capacity - * 02h 18h 128 Mb capacity - * 02h 19h 256 Mb capacity - * 03h 4Dh Full size of the RDID output (ignored) - * 04h 00h FS: 256-kB physical sectors - * 04h 01h FS: 64-kB physical sectors - * 04h 00h FL: 256-kB physical sectors - * 04h 01h FL: Mix of 64-kB and 4KB overlayed sectors - * 05h 80h FL family - * 05h 81h FS family - * - * Need to use bytes 1, 2, 4, and 5 to properly identify one of eight - * possible chips: - * - * 2 types * 2 possible sizes * 2 possible sector layouts - * - */ - memcpy(model_id.array, dev_id + 1, 2); - memcpy(model_id.array + 2, dev_id + 4, 2); - if (be_to_cpu32(model_id.whole) == flash->chip->model_id) - return 1; - } - } - - return 0; -} - -/* Used for Spansion/Cypress S25F chips */ -static int s25f_legacy_software_reset(struct flashctx *flash) -{ - int result; - struct spi_command cmds[] = { - { - .writecnt = 1, - .writearr = (const unsigned char[]){ CMD_RSTEN }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 1, - .writearr = (const unsigned char[]){ 0xf0 }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 0, - .writearr = NULL, - .readcnt = 0, - .readarr = NULL, - }}; - - result = spi_send_multicommand(flash, cmds); - if (result) { - msg_cerr("%s failed during command execution\n", __func__); - return result; - } - /* Reset takes 35us according to data-sheet, double that for safety */ - programmer_delay(T_RPH * 2); - - return 0; -} - -/* Only for Spansion S25FS chips, where legacy reset is disabled by default */ -int s25fs_software_reset(struct flashctx *flash) -{ - int result; - struct spi_command cmds[] = { - { - .writecnt = 1, - .writearr = (const unsigned char[]){ CMD_RSTEN }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 1, - .writearr = (const unsigned char[]){ CMD_RST }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 0, - .writearr = NULL, - .readcnt = 0, - .readarr = NULL, - }}; - - msg_cdbg("Force resetting SPI chip.\n"); - result = spi_send_multicommand(flash, cmds); - if (result) { - msg_cerr("%s failed during command execution\n", __func__); - return result; - } - - programmer_delay(T_RPH * 2); - - return 0; -} - -int spi_poll_wip(struct flashctx *const flash, const unsigned int poll_delay) -{ - uint8_t status_reg = spi_read_status_register(flash); - + /* FIXME: We can't tell if spi_read_status_register() failed. */ /* FIXME: We don't time out. */ - while (status_reg & SPI_SR_WIP) { - /* - * The WIP bit on S25F chips remains set to 1 if erase or - * programming errors occur, so we must check for those - * errors here. If an error is encountered, do a software - * reset to clear WIP and other volatile bits, otherwise - * the chip will be unresponsive to further commands. - */ - if (status_reg & SPI_SR_ERA_ERR) { - msg_cerr("Erase error occurred\n"); - s25f_legacy_software_reset(flash); - return -1; - } - if (status_reg & (1 << 6)) { - msg_cerr("Programming error occurred\n"); - s25f_legacy_software_reset(flash); - return -1; - } + while (spi_read_status_register(flash) & SPI_SR_WIP) programmer_delay(poll_delay); - status_reg = spi_read_status_register(flash); - } - + /* FIXME: Check the status register for errors. */ return 0; }
@@ -629,73 +486,6 @@ return spi_write_cmd(flash, 0xd8, false, addr, NULL, 0, 100 * 1000); }
-/* Used on Spansion/Cypress S25FS chips */ -int s25fs_block_erase_d8(struct flashctx *flash, - unsigned int addr, unsigned int blocklen) -{ - unsigned char cfg; - int result; - static int cr3nv_checked = 0; - - struct spi_command erase_cmds[] = { - { - .writecnt = JEDEC_WREN_OUTSIZE, - .writearr = (const unsigned char[]){ JEDEC_WREN }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = JEDEC_BE_D8_OUTSIZE, - .writearr = (const unsigned char[]){ - JEDEC_BE_D8, - (addr >> 16) & 0xff, - (addr >> 8) & 0xff, - (addr & 0xff) - }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 0, - .writearr = NULL, - .readcnt = 0, - .readarr = NULL, - }}; - - /* Check if hybrid sector architecture is in use and, if so, - * switch to uniform sectors. */ - if (!cr3nv_checked) { - cfg = s25fs_read_cr(flash, CR3NV_ADDR); - if (!(cfg & CR3NV_20H_NV)) { - s25fs_write_cr(flash, CR3NV_ADDR, cfg | CR3NV_20H_NV); - s25fs_software_reset(flash); - - cfg = s25fs_read_cr(flash, CR3NV_ADDR); - if (!(cfg & CR3NV_20H_NV)) { - msg_cerr("%s: Unable to enable uniform " - "block sizes.\n", __func__); - return 1; - } - - msg_cdbg("\n%s: CR3NV updated (0x%02x -> 0x%02x)\n", - __func__, cfg, - s25fs_read_cr(flash, CR3NV_ADDR)); - /* Restore CR3V when flashrom exits */ - register_chip_restore(s25fs_restore_cr3nv, flash, cfg); - } - - cr3nv_checked = 1; - } - - result = spi_send_multicommand(flash, erase_cmds); - if (result) { - msg_cerr("%s failed during command execution at address 0x%x\n", - __func__, addr); - return result; - } - - programmer_delay(T_SE); - return spi_poll_wip(flash, 1000 * 10); -} - /* Block size is usually * 4k for PMC */ diff --git a/spi25_statusreg.c b/spi25_statusreg.c index aa574d5..8cd5a28 100644 --- a/spi25_statusreg.c +++ b/spi25_statusreg.c @@ -108,89 +108,6 @@ return readarr[0]; }
-static int spi_restore_status(struct flashctx *flash, uint8_t status) -{ - msg_cdbg("restoring chip status (0x%02x)\n", status); - return spi_write_status_register(flash, status); -} - -/* 'Read Any Register' used on Spansion/Cypress S25FS chips */ -int s25fs_read_cr(struct flashctx *const flash, uint32_t addr) -{ - int result; - uint8_t cfg; - /* By default, 8 dummy cycles are necessary for variable-latency - commands such as RDAR (see CR2NV[3:0]). */ - unsigned char read_cr_cmd[] = { - CMD_RDAR, - (addr >> 16) & 0xff, - (addr >> 8) & 0xff, - (addr & 0xff), - 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, - }; - - result = spi_send_command(flash, sizeof(read_cr_cmd), 1, read_cr_cmd, &cfg); - if (result) { - msg_cerr("%s failed during command execution at address 0x%x\n", - __func__, addr); - return -1; - } - - return cfg; -} - -/* 'Write Any Register' used on Spansion/Cypress S25FS chips */ -int s25fs_write_cr(struct flashctx *const flash, - uint32_t addr, uint8_t data) -{ - int result; - struct spi_command cmds[] = { - { - .writecnt = JEDEC_WREN_OUTSIZE, - .writearr = (const unsigned char[]){ JEDEC_WREN }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = CMD_WRAR_LEN, - .writearr = (const unsigned char[]){ - CMD_WRAR, - (addr >> 16) & 0xff, - (addr >> 8) & 0xff, - (addr & 0xff), - data - }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 0, - .writearr = NULL, - .readcnt = 0, - .readarr = NULL, - }}; - - result = spi_send_multicommand(flash, cmds); - if (result) { - msg_cerr("%s failed during command execution at address 0x%x\n", - __func__, addr); - return -1; - } - - programmer_delay(T_W); - return spi_poll_wip(flash, 1000 * 10); -} - -/* Used on Spansion/Cypress S25FS chips */ -int s25fs_restore_cr3nv(struct flashctx *const flash, uint8_t cfg) -{ - int ret = 0; - - msg_cdbg("Restoring CR3NV value to 0x%02x\n", cfg); - ret |= s25fs_write_cr(flash, CR3NV_ADDR, cfg); - ret |= s25fs_software_reset(flash); - return ret; -} - /* A generic block protection disable. * Tests if a protection is enabled with the block protection mask (bp_mask) and returns success otherwise. * Tests if the register bits are locked with the lock_mask (lock_mask). @@ -222,9 +139,6 @@ return 0; }
- /* restore status register content upon exit */ - register_chip_restore(spi_restore_status, flash, status); - msg_cdbg("Some block protection in effect, disabling... "); if ((status & lock_mask) != 0) { msg_cdbg("\n\tNeed to disable the register lock first... ");
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/flashrom/+/40626/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/40626/1//COMMIT_MSG@11 PS1, Line 11: most What broke for you Nico? A local test for me didn't break on a Intel platform
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/40626/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/40626/1//COMMIT_MSG@11 PS1, Line 11: most
What broke for you Nico? A local test for me didn't break on a Intel platform
I was able to locally reproduce the failure with a Winbond, Thanks for picking this up quickly!
It was reviewed it, just wasn't noticed due to the similar function names/files [`s25f_poll_status():s25f.c` / `spi_poll_wip():spi25.c`] and the specific chip I tested with locally.
I agree with the big and invasive comment though, I put too much trust in the commit message explanation for that.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Patch Set 1: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/40626/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/40626/1//COMMIT_MSG@11 PS1, Line 11: most
I was able to locally reproduce the failure with a Winbond, Thanks for picking this up quickly! […]
This is why I never reviewed it. I can't keep my attention span over such a large change...
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Patch Set 1:
Nico, can you rebase so we can get this merged please?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Patch Set 1:
Nico, can you rebase so we can get this merged please?
It doesn't apply cleanly, I don't have the time, sorry. Please take over if you want.
Edward O'Callaghan has uploaded a new patch set (#2) to the change originally created by Nico Huber. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Revert "flashchips: port S25FS(128S) chip from chromiumos"
This reverts commit a3519561bd0fb44153bb376322b799000657576f.
Breaks support for most SPI flash chips. It's too big and too invasive to be reviewed as a single commit.
The changes to `spi_poll_wip():spi25.c` were not noticed in the original review that were from the similarly named function and file `s25f_poll_status():s25f.c` in the downstream Chromium fork.
V.2: Rebase and rephrase commit msg to reflect how the issue slipped in.
Change-Id: Id2a4593bdb654f8a26957d69d52189ce61621d93 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M chipdrivers.h M flash.h M flashchips.c M flashchips.h M flashrom.c M spi.h M spi25.c M spi25_statusreg.c 8 files changed, 6 insertions(+), 415 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/26/40626/2
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Patch Set 2:
Patch Set 1:
Nico, can you rebase so we can get this merged please?
It doesn't apply cleanly, I don't have the time, sorry. Please take over if you want.
Of course Nico. Thanks for taking the time for the original patch! I hope you don't mind, I modified the commit message slightly to reflect how the issue got in and to note where the issue came from in the original patch.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/flashrom/+/40626/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/40626/1//COMMIT_MSG@11 PS1, Line 11: most
This is why I never reviewed it. I can't keep my attention span over such a large change...
.
Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Patch Set 2: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Patch Set 2: Code-Review+2
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/40626 )
Change subject: Revert "flashchips: port S25FS(128S) chip from chromiumos" ......................................................................
Revert "flashchips: port S25FS(128S) chip from chromiumos"
This reverts commit a3519561bd0fb44153bb376322b799000657576f.
Breaks support for most SPI flash chips. It's too big and too invasive to be reviewed as a single commit.
The changes to `spi_poll_wip():spi25.c` were not noticed in the original review that were from the similarly named function and file `s25f_poll_status():s25f.c` in the downstream Chromium fork.
V.2: Rebase and rephrase commit msg to reflect how the issue slipped in.
Change-Id: Id2a4593bdb654f8a26957d69d52189ce61621d93 Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/40626 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Shiyu Sun sshiyu@google.com Reviewed-by: Angel Pons th3fanbus@gmail.com --- M chipdrivers.h M flash.h M flashchips.c M flashchips.h M flashrom.c M spi.h M spi25.c M spi25_statusreg.c 8 files changed, 6 insertions(+), 415 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Shiyu Sun: Looks good to me, but someone else must approve
diff --git a/chipdrivers.h b/chipdrivers.h index 04963c2..3c7d146 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -35,9 +35,6 @@ int probe_spi_res2(struct flashctx *flash); int probe_spi_res3(struct flashctx *flash); int probe_spi_at25f(struct flashctx *flash); -int probe_spi_big_spansion(struct flashctx *flash); -int s25fs_software_reset(struct flashctx *flash); -int spi_poll_wip(struct flashctx *const flash, const unsigned int poll_delay); int spi_write_enable(struct flashctx *flash); int spi_write_disable(struct flashctx *flash); int spi_block_erase_20(struct flashctx *flash, unsigned int addr, unsigned int blocklen); @@ -52,7 +49,6 @@ int spi_block_erase_c7(struct flashctx *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_d7(struct flashctx *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_d8(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int s25fs_block_erase_d8(struct flashctx *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_db(struct flashctx *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_dc(struct flashctx *flash, unsigned int addr, unsigned int blocklen); erasefunc_t *spi_get_erasefn_from_opcode(uint8_t opcode); @@ -64,12 +60,10 @@ int spi_exit_4ba(struct flashctx *flash); int spi_set_extended_address(struct flashctx *, uint8_t addr_high);
+ /* spi25_statusreg.c */ uint8_t spi_read_status_register(const struct flashctx *flash); int spi_write_status_register(const struct flashctx *flash, int status); -int s25fs_read_cr(struct flashctx *const flash, uint32_t addr); -int s25fs_write_cr(struct flashctx *const flash, uint32_t addr, uint8_t data); -int s25fs_restore_cr3nv(struct flashctx *const flash, uint8_t cfg); void spi_prettyprint_status_register_bit(uint8_t status, int bit); int spi_prettyprint_status_register_plain(struct flashctx *flash); int spi_prettyprint_status_register_default_welwip(struct flashctx *flash); diff --git a/flash.h b/flash.h index 1278e86..2f0143b 100644 --- a/flash.h +++ b/flash.h @@ -54,8 +54,6 @@ #define PRIxPTR_WIDTH ((int)(sizeof(uintptr_t)*2))
int register_shutdown(int (*function) (void *data), void *data); -#define CHIP_RESTORE_CALLBACK int (*func) (struct flashrom_flashctx *flash, uint8_t status) -int register_chip_restore(CHIP_RESTORE_CALLBACK, struct flashrom_flashctx *flash, uint8_t status); int shutdown_free(void *data); void *programmer_map_flash_region(const char *descr, uintptr_t phys_addr, size_t len); void programmer_unmap_flash_region(void *virt_addr, size_t len); @@ -209,7 +207,6 @@ SPI_EDI = 1, } spi_cmd_set;
- int (*reset) (struct flashctx *flash); int (*probe) (struct flashctx *flash);
/* Delay after "enter/exit ID mode" commands in microseconds. diff --git a/flashchips.c b/flashchips.c index 974ba81..b4006df 100644 --- a/flashchips.c +++ b/flashchips.c @@ -49,7 +49,6 @@ * .eraseblocks[] = Array of { blocksize, blockcount } * .block_erase = Block erase function * } - * .reset = Reset Chip * .printlock = Chip lock status function * .unlock = Chip unlock function * .write = Chip write function @@ -15826,69 +15825,6 @@
{ .vendor = "Spansion", - .name = "S25FS128S Large Sectors", - .bustype = BUS_SPI, - .manufacture_id = SPANSION_ID, - .model_id = SPANSION_S25FS128S_L, - .total_size = 16384, - .page_size = 256, - .feature_bits = FEATURE_WRSR_WREN, - .tested = TEST_UNTESTED, - .probe = probe_spi_big_spansion, - .probe_timing = TIMING_ZERO, - .block_erasers = - { - { - .eraseblocks = { {64 * 1024, 256} }, - .block_erase = s25fs_block_erase_d8, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_60, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_c7, - }, - }, - .unlock = spi_disable_blockprotect, - .write = spi_chip_write_256, - .read = spi_chip_read, - .voltage = {1700, 2000}, - }, - - { - .vendor = "Spansion", - .name = "S25FS128S Small Sectors", - .bustype = BUS_SPI, - .manufacture_id = SPANSION_ID, - .model_id = SPANSION_S25FS128S_S, - .total_size = 16384, - .page_size = 256, - .feature_bits = FEATURE_WRSR_WREN, - .tested = TEST_OK_PREW, - .probe = probe_spi_big_spansion, - .probe_timing = TIMING_ZERO, - .block_erasers = - { - { - .eraseblocks = { {64 * 1024, 256} }, - .block_erase = s25fs_block_erase_d8, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_60, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_c7, - }, - }, - .reset = s25fs_software_reset, - .unlock = spi_disable_blockprotect, - .write = spi_chip_write_256, - .read = spi_chip_read, - .voltage = {1700, 2000}, - }, - - { - .vendor = "Spansion", .name = "S25FL129P......1", /* uniform 256 kB sectors */ .bustype = BUS_SPI, .manufacture_id = SPANSION_ID, diff --git a/flashchips.h b/flashchips.h index 0c77d1d..e5ef390 100644 --- a/flashchips.h +++ b/flashchips.h @@ -642,8 +642,6 @@ #define SPANSION_S25FL032A 0x0215 /* Same as S25FL032P, but the latter supports EDI and CFI */ #define SPANSION_S25FL064A 0x0216 /* Same as S25FL064P, but the latter supports EDI and CFI */ #define SPANSION_S25FL128 0x2018 /* Same ID for various S25FL127S, S25FL128P, S25FL128S and S25FL129P (including dual-die S70FL256P) variants (EDI supported) */ -#define SPANSION_S25FS128S_L 0x20180081 /* Large sectors. */ -#define SPANSION_S25FS128S_S 0x20180181 /* Small sectors. */ #define SPANSION_S25FL256 0x0219 #define SPANSION_S25FL512 0x0220 #define SPANSION_S25FL204 0x4013 diff --git a/flashrom.c b/flashrom.c index 4224637..07ce734 100644 --- a/flashrom.c +++ b/flashrom.c @@ -500,14 +500,6 @@ {0}, /* This entry corresponds to PROGRAMMER_INVALID. */ };
-#define CHIP_RESTORE_MAXFN 4 -static int chip_restore_fn_count = 0; -static struct chip_restore_func_data { - CHIP_RESTORE_CALLBACK; - struct flashctx *flash; - uint8_t status; -} chip_restore_fn[CHIP_RESTORE_MAXFN]; - #define SHUTDOWN_MAXFN 32 static int shutdown_fn_count = 0; /** @private */ @@ -558,23 +550,6 @@ return 0; }
-//int register_chip_restore(int (*function) (void *data), void *data) -int register_chip_restore(CHIP_RESTORE_CALLBACK, - struct flashctx *flash, uint8_t status) -{ - if (chip_restore_fn_count >= CHIP_RESTORE_MAXFN) { - msg_perr("Tried to register more than %i chip restore" - " functions.\n", CHIP_RESTORE_MAXFN); - return 1; - } - chip_restore_fn[chip_restore_fn_count].func = func; /* from macro */ - chip_restore_fn[chip_restore_fn_count].flash = flash; - chip_restore_fn[chip_restore_fn_count].status = status; - chip_restore_fn_count++; - - return 0; -} - int programmer_init(enum programmer prog, const char *param) { int ret; diff --git a/spi.h b/spi.h index 12063d7..3f45038 100644 --- a/spi.h +++ b/spi.h @@ -101,7 +101,7 @@ #define JEDEC_BE_C4_OUTSIZE 0x04 #define JEDEC_BE_C4_INSIZE 0x00
-/* Block Erase 0xd8 is supported by EON/Macronix/Spansion chips. */ +/* Block Erase 0xd8 is supported by EON/Macronix chips. */ #define JEDEC_BE_D8 0xd8 #define JEDEC_BE_D8_OUTSIZE 0x04 #define JEDEC_BE_D8_INSIZE 0x00 @@ -116,18 +116,6 @@ #define JEDEC_SE_OUTSIZE 0x04 #define JEDEC_SE_INSIZE 0x00
-/* RADR, WRAR, RSTEN, RST & CR3NV OPs and timers on Spansion S25FS chips */ -#define CMD_RDAR 0x65 -#define CMD_WRAR 0x71 -#define CMD_WRAR_LEN 5 -#define CMD_RSTEN 0x66 -#define CMD_RST 0x99 -#define CR3NV_ADDR 0x000004 -#define CR3NV_20H_NV (1 << 3) -#define T_W 145 * 1000 /* NV register write time */ -#define T_RPH 35 /* Reset pulse hold time */ -#define T_SE 145 * 1000 /* Sector Erase Time */ - /* Page Erase 0xDB */ #define JEDEC_PE 0xDB #define JEDEC_PE_OUTSIZE 0x04 @@ -141,7 +129,6 @@ /* Status Register Bits */ #define SPI_SR_WIP (0x01 << 0) #define SPI_SR_WEL (0x01 << 1) -#define SPI_SR_ERA_ERR (0x01 << 5) #define SPI_SR_AAI (0x01 << 6)
/* Write Status Enable */ diff --git a/spi25.c b/spi25.c index 16d7f1b..0a939e7 100644 --- a/spi25.c +++ b/spi25.c @@ -25,7 +25,6 @@ #include "flashchips.h" #include "chipdrivers.h" #include "programmer.h" -#include "hwaccess.h" #include "spi.h"
static int spi_rdid(struct flashctx *flash, unsigned char *readarr, int bytes) @@ -285,155 +284,13 @@ return 0; }
-/* Used for probing 'big' Spansion/Cypress S25FS chips */ -int probe_spi_big_spansion(struct flashctx *flash) +static int spi_poll_wip(struct flashctx *const flash, const unsigned int poll_delay) { - static const unsigned char cmd = JEDEC_RDID; - int ret; - unsigned char dev_id[6]; /* We care only about 6 first bytes */ - - ret = spi_send_command(flash, sizeof(cmd), sizeof(dev_id), &cmd, dev_id); - - if (!ret) { - unsigned long i; - - for (i = 0; i < sizeof(dev_id); i++) - msg_gdbg(" 0x%02x", dev_id[i]); - msg_gdbg(".\n"); - - if (dev_id[0] == flash->chip->manufacture_id) { - union { - uint8_t array[4]; - uint32_t whole; - } model_id; - - /* - * The structure of the RDID output is as follows: - * - * offset value meaning - * 00h 01h Manufacturer ID for Spansion - * 01h 20h 128 Mb capacity - * 01h 02h 256 Mb capacity - * 02h 18h 128 Mb capacity - * 02h 19h 256 Mb capacity - * 03h 4Dh Full size of the RDID output (ignored) - * 04h 00h FS: 256-kB physical sectors - * 04h 01h FS: 64-kB physical sectors - * 04h 00h FL: 256-kB physical sectors - * 04h 01h FL: Mix of 64-kB and 4KB overlayed sectors - * 05h 80h FL family - * 05h 81h FS family - * - * Need to use bytes 1, 2, 4, and 5 to properly identify one of eight - * possible chips: - * - * 2 types * 2 possible sizes * 2 possible sector layouts - * - */ - memcpy(model_id.array, dev_id + 1, 2); - memcpy(model_id.array + 2, dev_id + 4, 2); - if (be_to_cpu32(model_id.whole) == flash->chip->model_id) - return 1; - } - } - - return 0; -} - -/* Used for Spansion/Cypress S25F chips */ -static int s25f_legacy_software_reset(struct flashctx *flash) -{ - int result; - struct spi_command cmds[] = { - { - .writecnt = 1, - .writearr = (const unsigned char[]){ CMD_RSTEN }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 1, - .writearr = (const unsigned char[]){ 0xf0 }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 0, - .writearr = NULL, - .readcnt = 0, - .readarr = NULL, - }}; - - result = spi_send_multicommand(flash, cmds); - if (result) { - msg_cerr("%s failed during command execution\n", __func__); - return result; - } - /* Reset takes 35us according to data-sheet, double that for safety */ - programmer_delay(T_RPH * 2); - - return 0; -} - -/* Only for Spansion S25FS chips, where legacy reset is disabled by default */ -int s25fs_software_reset(struct flashctx *flash) -{ - int result; - struct spi_command cmds[] = { - { - .writecnt = 1, - .writearr = (const unsigned char[]){ CMD_RSTEN }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 1, - .writearr = (const unsigned char[]){ CMD_RST }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 0, - .writearr = NULL, - .readcnt = 0, - .readarr = NULL, - }}; - - msg_cdbg("Force resetting SPI chip.\n"); - result = spi_send_multicommand(flash, cmds); - if (result) { - msg_cerr("%s failed during command execution\n", __func__); - return result; - } - - programmer_delay(T_RPH * 2); - - return 0; -} - -int spi_poll_wip(struct flashctx *const flash, const unsigned int poll_delay) -{ - uint8_t status_reg = spi_read_status_register(flash); - + /* FIXME: We can't tell if spi_read_status_register() failed. */ /* FIXME: We don't time out. */ - while (status_reg & SPI_SR_WIP) { - /* - * The WIP bit on S25F chips remains set to 1 if erase or - * programming errors occur, so we must check for those - * errors here. If an error is encountered, do a software - * reset to clear WIP and other volatile bits, otherwise - * the chip will be unresponsive to further commands. - */ - if (status_reg & SPI_SR_ERA_ERR) { - msg_cerr("Erase error occurred\n"); - s25f_legacy_software_reset(flash); - return -1; - } - if (status_reg & (1 << 6)) { - msg_cerr("Programming error occurred\n"); - s25f_legacy_software_reset(flash); - return -1; - } + while (spi_read_status_register(flash) & SPI_SR_WIP) programmer_delay(poll_delay); - status_reg = spi_read_status_register(flash); - } - + /* FIXME: Check the status register for errors. */ return 0; }
@@ -631,73 +488,6 @@ return spi_write_cmd(flash, 0xd8, false, addr, NULL, 0, 100 * 1000); }
-/* Used on Spansion/Cypress S25FS chips */ -int s25fs_block_erase_d8(struct flashctx *flash, - unsigned int addr, unsigned int blocklen) -{ - unsigned char cfg; - int result; - static int cr3nv_checked = 0; - - struct spi_command erase_cmds[] = { - { - .writecnt = JEDEC_WREN_OUTSIZE, - .writearr = (const unsigned char[]){ JEDEC_WREN }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = JEDEC_BE_D8_OUTSIZE, - .writearr = (const unsigned char[]){ - JEDEC_BE_D8, - (addr >> 16) & 0xff, - (addr >> 8) & 0xff, - (addr & 0xff) - }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 0, - .writearr = NULL, - .readcnt = 0, - .readarr = NULL, - }}; - - /* Check if hybrid sector architecture is in use and, if so, - * switch to uniform sectors. */ - if (!cr3nv_checked) { - cfg = s25fs_read_cr(flash, CR3NV_ADDR); - if (!(cfg & CR3NV_20H_NV)) { - s25fs_write_cr(flash, CR3NV_ADDR, cfg | CR3NV_20H_NV); - s25fs_software_reset(flash); - - cfg = s25fs_read_cr(flash, CR3NV_ADDR); - if (!(cfg & CR3NV_20H_NV)) { - msg_cerr("%s: Unable to enable uniform " - "block sizes.\n", __func__); - return 1; - } - - msg_cdbg("\n%s: CR3NV updated (0x%02x -> 0x%02x)\n", - __func__, cfg, - s25fs_read_cr(flash, CR3NV_ADDR)); - /* Restore CR3V when flashrom exits */ - register_chip_restore(s25fs_restore_cr3nv, flash, cfg); - } - - cr3nv_checked = 1; - } - - result = spi_send_multicommand(flash, erase_cmds); - if (result) { - msg_cerr("%s failed during command execution at address 0x%x\n", - __func__, addr); - return result; - } - - programmer_delay(T_SE); - return spi_poll_wip(flash, 1000 * 10); -} - /* Block size is usually * 4k for PMC */ diff --git a/spi25_statusreg.c b/spi25_statusreg.c index 083a832..4b9b2a9 100644 --- a/spi25_statusreg.c +++ b/spi25_statusreg.c @@ -108,89 +108,6 @@ return readarr[0]; }
-static int spi_restore_status(struct flashctx *flash, uint8_t status) -{ - msg_cdbg("restoring chip status (0x%02x)\n", status); - return spi_write_status_register(flash, status); -} - -/* 'Read Any Register' used on Spansion/Cypress S25FS chips */ -int s25fs_read_cr(struct flashctx *const flash, uint32_t addr) -{ - int result; - uint8_t cfg; - /* By default, 8 dummy cycles are necessary for variable-latency - commands such as RDAR (see CR2NV[3:0]). */ - unsigned char read_cr_cmd[] = { - CMD_RDAR, - (addr >> 16) & 0xff, - (addr >> 8) & 0xff, - (addr & 0xff), - 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, - }; - - result = spi_send_command(flash, sizeof(read_cr_cmd), 1, read_cr_cmd, &cfg); - if (result) { - msg_cerr("%s failed during command execution at address 0x%x\n", - __func__, addr); - return -1; - } - - return cfg; -} - -/* 'Write Any Register' used on Spansion/Cypress S25FS chips */ -int s25fs_write_cr(struct flashctx *const flash, - uint32_t addr, uint8_t data) -{ - int result; - struct spi_command cmds[] = { - { - .writecnt = JEDEC_WREN_OUTSIZE, - .writearr = (const unsigned char[]){ JEDEC_WREN }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = CMD_WRAR_LEN, - .writearr = (const unsigned char[]){ - CMD_WRAR, - (addr >> 16) & 0xff, - (addr >> 8) & 0xff, - (addr & 0xff), - data - }, - .readcnt = 0, - .readarr = NULL, - }, { - .writecnt = 0, - .writearr = NULL, - .readcnt = 0, - .readarr = NULL, - }}; - - result = spi_send_multicommand(flash, cmds); - if (result) { - msg_cerr("%s failed during command execution at address 0x%x\n", - __func__, addr); - return -1; - } - - programmer_delay(T_W); - return spi_poll_wip(flash, 1000 * 10); -} - -/* Used on Spansion/Cypress S25FS chips */ -int s25fs_restore_cr3nv(struct flashctx *const flash, uint8_t cfg) -{ - int ret = 0; - - msg_cdbg("Restoring CR3NV value to 0x%02x\n", cfg); - ret |= s25fs_write_cr(flash, CR3NV_ADDR, cfg); - ret |= s25fs_software_reset(flash); - return ret; -} - /* A generic block protection disable. * Tests if a protection is enabled with the block protection mask (bp_mask) and returns success otherwise. * Tests if the register bits are locked with the lock_mask (lock_mask). @@ -222,9 +139,6 @@ return 0; }
- /* restore status register content upon exit */ - register_chip_restore(spi_restore_status, flash, status); - msg_cdbg("Some block protection in effect, disabling... "); if ((status & lock_mask) != 0) { msg_cdbg("\n\tNeed to disable the register lock first... ");
-
Angel Pons (Code Review) -
Edward O'Callaghan (Code Review)
-
Nico Huber (Code Review)
-
Shiyu Sun (Code Review)