Edward O'Callaghan submitted this change.

View Change

Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Shiyu Sun: Looks good to me, but someone else must approve
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(-)

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... ");

To view, visit change 40626. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id2a4593bdb654f8a26957d69d52189ce61621d93
Gerrit-Change-Number: 40626
Gerrit-PatchSet: 3
Gerrit-Owner: Nico Huber <nico.h@gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org>
Gerrit-Reviewer: Hung-Te Lin <hungte@chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net>
Gerrit-Reviewer: Samir Ibradžić <sibradzic@gmail.com>
Gerrit-Reviewer: Shiyu Sun <sshiyu@google.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org>
Gerrit-MessageType: merged