October 2021 - flashrom-gerrit

Change in flashrom[master]: [RFC] spi25_statusreg: make status register functions generic
by Nikolai Artemiev (Code Review) 20 Oct '21

20 Oct '21

Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/58475 ) Change subject: [RFC] spi25_statusreg: make status register functions generic ...................................................................... [RFC] spi25_statusreg: make status register functions generic This patch adds new spi_{read,write}_register() functions that take the source/destination register as an argument. Currently they still only accept the STATUS1 register, support for other registers will be added in following patches. Since we're refactoring things, this commit also makes spi_read_register() return an error code, making it possible to identify error conditions that spi_read_status_register() concealed. BUG=b:195381327,b:153800563 TEST=builds, tested flashrom/wp commands at end of patch chain BRANCH=none Change-Id: I0a3951bbf993f2d8d830143b29d3ce16cc6901d7 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M chipdrivers.h M flash.h M spi25_statusreg.c 3 files changed, 80 insertions(+), 29 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/75/58475/1 diff --git a/chipdrivers.h b/chipdrivers.h index e1d6aa9..ea8d480 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -62,8 +62,10 @@ /* spi25_statusreg.c */ +/* FIXME: replace spi_read_status_register() calls with spi_read_register() */ uint8_t spi_read_status_register(const struct flashctx *flash); -int spi_write_status_register(const struct flashctx *flash, int status); +int spi_read_register(const struct flashctx *flash, enum flash_reg reg, uint8_t *value); +int spi_write_register(const struct flashctx *flash, enum flash_reg reg, uint8_t value); 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 18e1a57..c3f99c9 100644 --- a/flash.h +++ b/flash.h @@ -168,6 +168,15 @@ #define flashctx flashrom_flashctx /* TODO: Agree on a name and convert all occurences. */ typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen); +enum flash_reg { + INVALID_REG = 0, + + STATUS1, + STATUS2, + CONFIG1, +}; +#define MAX_REGISTERS 4 + struct flashchip { const char *vendor; const char *name; diff --git a/spi25_statusreg.c b/spi25_statusreg.c index a0b0fcf..8e8eead 100644 --- a/spi25_statusreg.c +++ b/spi25_statusreg.c @@ -22,24 +22,27 @@ #include "spi.h" /* === Generic functions === */ -static int spi_write_status_register_flag(const struct flashctx *flash, int status, const unsigned char enable_opcode) +static int spi_write_register_flag( + const struct flashctx *flash, + uint8_t enable_opcode, + uint8_t *write_cmd, + size_t write_cmd_len) { - int result; - int i = 0; /* * WRSR requires either EWSR or WREN depending on chip type. * The code below relies on the fact hat EWSR and WREN have the same * INSIZE and OUTSIZE. */ + struct spi_command cmds[] = { { .writecnt = JEDEC_WREN_OUTSIZE, - .writearr = (const unsigned char[]){ enable_opcode }, + .writearr = &enable_opcode, .readcnt = 0, .readarr = NULL, }, { - .writecnt = JEDEC_WRSR_OUTSIZE, - .writearr = (const unsigned char[]){ JEDEC_WRSR, (unsigned char) status }, + .writecnt = write_cmd_len, + .writearr = write_cmd, .readcnt = 0, .readarr = NULL, }, { @@ -49,7 +52,7 @@ .readarr = NULL, }}; - result = spi_send_multicommand(flash, cmds); + int result = spi_send_multicommand(flash, cmds); if (result) { msg_cerr("%s failed during command execution\n", __func__); /* No point in waiting for the command to complete if execution @@ -62,6 +65,7 @@ * allow running RDSR only once. Therefore pick an initial delay of * 100 ms, then wait in 10 ms steps until a total of 5 s have elapsed. */ + int i = 0; programmer_delay(100 * 1000); while (spi_read_status_register(flash) & SPI_SR_WIP) { if (++i > 490) { @@ -73,47 +77,83 @@ return 0; } -int spi_write_status_register(const struct flashctx *flash, int status) +int spi_write_register(const struct flashctx *flash, enum flash_reg reg, uint8_t value) { int feature_bits = flash->chip->feature_bits; - int ret = 1; + + uint8_t write_cmd[3]; + size_t write_cmd_len = 0; + + /* Create SPI write command sequence based on the destination register + * and the chip's supported command set. */ + if (reg == STATUS1) { + write_cmd[0] = JEDEC_WRSR; + write_cmd[1] = value; + write_cmd_len = 2; + } + if(write_cmd_len == 0) { + msg_cerr("Cannot write register: not supported by chip\n"); + return 1; + } if (!(feature_bits & (FEATURE_WRSR_WREN | FEATURE_WRSR_EWSR))) { msg_cdbg("Missing status register write definition, assuming " "EWSR is needed\n"); feature_bits |= FEATURE_WRSR_EWSR; } + + int ret = 1; if (feature_bits & FEATURE_WRSR_WREN) - ret = spi_write_status_register_flag(flash, status, JEDEC_WREN); + ret = spi_write_register_flag(flash, JEDEC_WREN, write_cmd, write_cmd_len); if (ret && (feature_bits & FEATURE_WRSR_EWSR)) - ret = spi_write_status_register_flag(flash, status, JEDEC_EWSR); + ret = spi_write_register_flag(flash, JEDEC_EWSR, write_cmd, write_cmd_len); return ret; } -uint8_t spi_read_status_register(const struct flashctx *flash) +int spi_read_register(const struct flashctx *flash, enum flash_reg reg, uint8_t *value) { - static const unsigned char cmd[JEDEC_RDSR_OUTSIZE] = { JEDEC_RDSR }; - /* FIXME: No workarounds for driver/hardware bugs in generic code. */ - unsigned char readarr[2]; /* JEDEC_RDSR_INSIZE=1 but wbsio needs 2 */ - int ret; + uint8_t read_cmd; - /* Read Status Register */ - ret = spi_send_command(flash, sizeof(cmd), sizeof(readarr), cmd, readarr); - if (ret) { - msg_cerr("RDSR failed!\n"); - /* FIXME: We should propagate the error. */ - return 0; + if (reg == STATUS1) { + read_cmd = JEDEC_RDSR; + } + else { + msg_cerr("Cannot read register: not supported by chip\n"); + return 1; } - return readarr[0]; + /* FIXME: No workarounds for driver/hardware bugs in generic code. */ + /* JEDEC_RDSR_INSIZE=1 but wbsio needs 2 */ + uint8_t readarr[2]; + + int ret = spi_send_command( + flash, sizeof(read_cmd), sizeof(readarr), + &read_cmd, readarr); + if (ret) { + msg_cerr("Register read failed!\n"); + return ret; + } + + *value = readarr[0]; + return 0; +} + + +uint8_t spi_read_status_register(const struct flashctx *flash) +{ + uint8_t status; + /* FIXME: We should propagate the error. */ + spi_read_register(flash, STATUS1, &status); + return status; } 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); + return spi_write_register(flash, STATUS1, status); } + /* 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). @@ -156,9 +196,9 @@ return 1; } /* All bits except the register lock bit (often called SPRL, SRWD, WPEN) are readonly. */ - result = spi_write_status_register(flash, status & ~lock_mask); + result = spi_write_register(flash, STATUS1, status & ~lock_mask); if (result) { - msg_cerr("spi_write_status_register failed.\n"); + msg_cerr("spi_write_register failed.\n"); return result; } status = spi_read_status_register(flash); @@ -169,9 +209,9 @@ msg_cdbg("done.\n"); } /* Global unprotect. Make sure to mask the register lock bit as well. */ - result = spi_write_status_register(flash, status & ~(bp_mask | lock_mask) & unprotect_mask); + result = spi_write_register(flash, STATUS1, status & ~(bp_mask | lock_mask) & unprotect_mask); if (result) { - msg_cerr("spi_write_status_register failed.\n"); + msg_cerr("spi_write_register failed.\n"); return result; } status = spi_read_status_register(flash); -- To view, visit https://review.coreboot.org/c/flashrom/+/58475 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0a3951bbf993f2d8d830143b29d3ce16cc6901d7 Gerrit-Change-Number: 58475 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: [RFC] writeprotect, cli_classic: delete old code, revise wp interface
by Nikolai Artemiev (Code Review) 20 Oct '21

20 Oct '21

Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/58474 ) Change subject: [RFC] writeprotect, cli_classic: delete old code, revise wp interface ...................................................................... [RFC] writeprotect, cli_classic: delete old code, revise wp interface This commit deletes most of the old writeprotect implementation as not much of it can be adapted. It also revises the writeprotect interface to increase consistency where possible and make it easier to expose through libflashrom. BUG=b:195381327,b:153800563 TEST=builds, all wp cmds tested at end of patch chain BRANCH=none Change-Id: I67e9b31f86465e5a8f7d3def637198671ee818a8 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M cli_classic.c M flash.h M writeprotect.c M writeprotect.h 4 files changed, 74 insertions(+), 423 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/58474/1 diff --git a/cli_classic.c b/cli_classic.c index d69b798..447b3a0 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -128,6 +128,21 @@ return true; } +static int parse_wp_mode(const char *wp_mode_opt, enum wp_mode *mode) +{ + if (!wp_mode_opt || !strcasecmp("hardware", wp_mode_opt)) { + *mode = WP_MODE_HARDWARE; + } else if (!strcasecmp("power_cycle", wp_mode_opt)) { + *mode = WP_MODE_POWER_CYCLE; + } else if (!strcasecmp("permanent", wp_mode_opt)) { + *mode = WP_MODE_PERMANENT; + } else { + msg_gerr("Error: Invalid WP mode: \"%s\"\n", wp_mode_opt); + return 1; + } + return 0; +} + static int parse_wp_range(unsigned int *start, unsigned int *len) { char *endptr = NULL, *token = NULL; @@ -673,9 +688,12 @@ goto out_shutdown; } - if (!(read_it | write_it | verify_it | erase_it | flash_name | flash_size - | set_wp_range | set_wp_region | set_wp_enable | - set_wp_disable | wp_status | wp_list | extract_it)) { + bool any_wp_op = + set_wp_range | set_wp_region | set_wp_enable | + set_wp_disable | wp_status | wp_list; + + if (!(read_it | write_it | verify_it | erase_it | flash_name | flash_size| + any_wp_op | extract_it)) { msg_ginfo("No operations were specified.\n"); goto out_shutdown; } @@ -691,13 +709,10 @@ goto out_shutdown; } - struct wp *wp = fill_flash->chip->wp; - if (set_wp_range || set_wp_region) { - if (!wp || !wp->set_range) { - msg_gerr("Error: write protect is not supported on this flash chip.\n"); - ret = 1; - goto out_shutdown; - } + if (any_wp_op && !wp_supported(fill_flash)) { + msg_gerr("Error: write protect is not supported on this flash chip.\n"); + ret = 1; + goto out_shutdown; } if (flash_name) { @@ -756,29 +771,19 @@ flashrom_layout_set(fill_flash, layout); if (wp_status) { - if (wp && wp->wp_status) { - ret |= wp->wp_status(fill_flash); - } else { - msg_gerr("Error: write protect is not supported on this flash chip.\n"); - ret = 1; - } + ret |= wp_print_status(fill_flash); goto out_release; } /* Note: set_wp_disable should be done before setting the range */ if (set_wp_disable) { - if (wp && wp->disable) { - ret |= wp->disable(fill_flash); - } else { - msg_gerr("Error: write protect is not supported on this flash chip.\n"); - ret = 1; - goto out_release; - } + ret |= wp_set_mode(fill_flash, WP_MODE_DISABLED); } /* Note: set_wp_range must happen before set_wp_enable */ if (set_wp_range) { - ret |= wp->set_range(fill_flash, wp_start, wp_len); + struct wp_range range = {0, wp_start, wp_len}; + ret |= wp_set_range(fill_flash, range); } if (set_wp_region && wp_region) { @@ -786,42 +791,23 @@ ret = 1; goto out_release; } - ret |= wp->set_range(fill_flash, wp_start, wp_len); - free(wp_region); + struct wp_range range = {0, wp_start, wp_len}; + ret |= wp_set_range(fill_flash, range); } if (!ret && set_wp_enable) { - enum wp_mode wp_mode; - - if (wp_mode_opt) - wp_mode = get_wp_mode(wp_mode_opt); - else - wp_mode = WP_MODE_HARDWARE; /* default */ - - if (wp_mode == WP_MODE_UNKNOWN) { - msg_gerr("Error: Invalid WP mode: \"%s\"\n", wp_mode_opt); + enum wp_mode mode; + if (parse_wp_mode(wp_mode_opt, &mode)) { ret = 1; goto out_release; } - if (wp && wp->enable) { - ret |= wp->enable(fill_flash, wp_mode); - } else { - msg_gerr("Error: write protect is not supported on this flash chip.\n"); - ret = 1; - goto out_release; - } + ret |= wp_set_mode(fill_flash, mode); } if (wp_list) { msg_ginfo("Valid write protection ranges:\n"); - if (wp && wp->list_ranges) { - ret |= wp->list_ranges(fill_flash); - } else { - msg_gerr("Error: write protect is not supported on this flash chip.\n"); - ret = 1; - } - goto out_release; + ret |= wp_list_ranges(fill_flash); } flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE, !!force); diff --git a/flash.h b/flash.h index 3baa076..18e1a57 100644 --- a/flash.h +++ b/flash.h @@ -247,7 +247,6 @@ /* SPI specific options (TODO: Make it a union in case other bustypes get specific options.) */ uint8_t wrea_override; /**< override opcode for write extended address register */ - struct wp *wp; }; typedef int (*chip_restore_fn_cb_t)(struct flashctx *flash, uint8_t status); diff --git a/writeprotect.c b/writeprotect.c index b26c121..1d907f6 100644 --- a/writeprotect.c +++ b/writeprotect.c @@ -15,380 +15,52 @@ * */ +#include <stdio.h> #include <stdlib.h> #include <string.h> -#include <strings.h> #include "flash.h" -#include "flashchips.h" #include "chipdrivers.h" -#include "spi.h" #include "writeprotect.h" -/* - * The following procedures rely on look-up tables to match the user-specified - * range with the chip's supported ranges. This turned out to be the most - * elegant approach since diferent flash chips use different levels of - * granularity and methods to determine protected ranges. In other words, - * be stupid and simple since clever arithmetic will not work for many chips. - */ - -struct wp_range { - unsigned int start; /* starting address */ - unsigned int len; /* len */ -}; - -enum bit_state { - OFF = 0, - ON = 1, - X = -1 /* don't care. Must be bigger than max # of bp. */ -}; - -/* - * Generic write-protection schema for 25-series SPI flash chips. This assumes - * there is a status register that contains one or more consecutive bits which - * determine which address range is protected. - */ - -struct status_register_layout { - int bp0_pos; /* position of BP0 */ - int bp_bits; /* number of block protect bits */ - int srp_pos; /* position of status register protect enable bit */ -}; - -/* - * The following ranges and functions are useful for representing the - * writeprotect schema in which there are typically 5 bits of - * relevant information stored in status register 1: - * m.sec: This bit indicates the units (sectors vs. blocks) - * m.tb: The top-bottom bit indicates if the affected range is at the top of - * the flash memory's address space or at the bottom. - * bp: Bitmask representing the number of affected sectors/blocks. - */ -struct wp_range_descriptor { - struct modifier_bits m; - unsigned int bp; /* block protect bitfield */ - struct wp_range range; -}; - -struct wp_context { - struct status_register_layout sr1; /* status register 1 */ - struct wp_range_descriptor *descrs; - - /* - * Some chips store modifier bits in one or more special control - * registers instead of the status register like many older SPI NOR - * flash chips did. get_modifier_bits() and set_modifier_bits() will do - * any chip-specific operations necessary to get/set these bit values. - */ - int (*get_modifier_bits)(const struct flashctx *flash, - struct modifier_bits *m); - int (*set_modifier_bits)(const struct flashctx *flash, - struct modifier_bits *m); -}; - -/* - * Mask to extract write-protect enable and range bits - * Status register 1: - * SRP0: bit 7 - * range(BP2-BP0): bit 4-2 - * range(BP3-BP0): bit 5-2 (large chips) - * Status register 2: - * SRP1: bit 1 - */ -#define MASK_WP_AREA (0x9C) -#define MASK_WP_AREA_LARGE (0x9C) -#define MASK_WP2_AREA (0x01) - -static uint8_t do_read_status(const struct flashctx *flash) +bool wp_supported(struct flashctx *flash) { - if (flash->chip->read_status) - return flash->chip->read_status(flash); - else - return spi_read_status_register(flash); + /* TODO */ + return false; } -static int do_write_status(const struct flashctx *flash, int status) +int wp_get_range(struct flashctx *flash, struct wp_range *range) { - if (flash->chip->write_status) - return flash->chip->write_status(flash, status); - else - return spi_write_status_register(flash, status); + /* TODO */ + return 1; } -enum wp_mode get_wp_mode(const char *mode_str) +int wp_set_range(struct flashctx *flash, const struct wp_range range) { - enum wp_mode wp_mode = WP_MODE_UNKNOWN; - - if (!strcasecmp(mode_str, "hardware")) - wp_mode = WP_MODE_HARDWARE; - else if (!strcasecmp(mode_str, "power_cycle")) - wp_mode = WP_MODE_POWER_CYCLE; - else if (!strcasecmp(mode_str, "permanent")) - wp_mode = WP_MODE_PERMANENT; - - return wp_mode; + /* TODO */ + return 1; } -/* Given a flash chip, this function returns its writeprotect info. */ -static int generic_range_table(const struct flashctx *flash, - struct wp_context **wp, - int *num_entries) +int wp_get_mode(struct flashctx *flash, enum wp_mode *mode) { - *wp = NULL; - *num_entries = 0; - - switch (flash->chip->manufacture_id) { - default: - msg_cerr("%s: flash vendor (0x%x) not found, aborting\n", - __func__, flash->chip->manufacture_id); - return -1; - } - - return 0; + /* TODO */ + return 1; } -static uint8_t generic_get_bp_mask(struct wp_context *wp) +int wp_set_mode(struct flashctx *flash, enum wp_mode mode) { - return ((1 << (wp->sr1.bp0_pos + wp->sr1.bp_bits)) - 1) ^ \ - ((1 << wp->sr1.bp0_pos) - 1); + /* TODO */ + return 1; } -static uint8_t generic_get_status_check_mask(struct wp_context *wp) +int wp_list_ranges(struct flashctx *flash) { - return generic_get_bp_mask(wp) | 1 << wp->sr1.srp_pos; + /* TODO */ + return 1; } -/* Given a [start, len], this function finds a block protect bit combination - * (if possible) and sets the corresponding bits in "status". Remaining bits - * are preserved. */ -static int generic_range_to_status(const struct flashctx *flash, - unsigned int start, unsigned int len, - uint8_t *status, uint8_t *check_mask) +int wp_print_status(struct flashctx *flash) { - struct wp_context *wp; - struct wp_range_descriptor *r; - int i, range_found = 0, num_entries; - uint8_t bp_mask; - - if (generic_range_table(flash, &wp, &num_entries)) - return -1; - - bp_mask = generic_get_bp_mask(wp); - - for (i = 0, r = &wp->descrs[0]; i < num_entries; i++, r++) { - msg_cspew("comparing range 0x%x 0x%x / 0x%x 0x%x\n", - start, len, r->range.start, r->range.len); - if ((start == r->range.start) && (len == r->range.len)) { - *status &= ~(bp_mask); - *status |= r->bp << (wp->sr1.bp0_pos); - - if (wp->set_modifier_bits) { - if (wp->set_modifier_bits(flash, &r->m) < 0) { - msg_cerr("error setting modifier bits for range.\n"); - return -1; - } - } - - range_found = 1; - break; - } - } - - if (!range_found) { - msg_cerr("%s: matching range not found\n", __func__); - return -1; - } - - *check_mask = generic_get_status_check_mask(wp); - return 0; + /* TODO */ + return 1; } - -static int generic_status_to_range(const struct flashctx *flash, - const uint8_t sr1, unsigned int *start, unsigned int *len) -{ - struct wp_context *wp; - struct wp_range_descriptor *r; - int num_entries, i, status_found = 0; - uint8_t sr1_bp; - struct modifier_bits m; - - if (generic_range_table(flash, &wp, &num_entries)) - return -1; - - /* modifier bits may be compared more than once, so get them here */ - if (wp->get_modifier_bits && wp->get_modifier_bits(flash, &m) < 0) - return -1; - - sr1_bp = (sr1 >> wp->sr1.bp0_pos) & ((1 << wp->sr1.bp_bits) - 1); - - for (i = 0, r = &wp->descrs[0]; i < num_entries; i++, r++) { - if (wp->get_modifier_bits) { - if (memcmp(&m, &r->m, sizeof(m))) - continue; - } - msg_cspew("comparing 0x%02x 0x%02x\n", sr1_bp, r->bp); - if (sr1_bp == r->bp) { - *start = r->range.start; - *len = r->range.len; - status_found = 1; - break; - } - } - - if (!status_found) { - msg_cerr("matching status not found\n"); - return -1; - } - - return 0; -} - -/* Given a [start, len], this function calls generic_range_to_status() to - * convert it to flash-chip-specific range bits, then sets into status register. - */ -static int generic_set_range(const struct flashctx *flash, - unsigned int start, unsigned int len) -{ - uint8_t status, expected, check_mask; - - status = do_read_status(flash); - msg_cdbg("%s: old status: 0x%02x\n", __func__, status); - - expected = status; /* preserve non-bp bits */ - if (generic_range_to_status(flash, start, len, &expected, &check_mask)) - return -1; - - do_write_status(flash, expected); - - status = do_read_status(flash); - msg_cdbg("%s: new status: 0x%02x\n", __func__, status); - if ((status & check_mask) != (expected & check_mask)) { - msg_cerr("expected=0x%02x, but actual=0x%02x. check mask=0x%02x\n", - expected, status, check_mask); - return 1; - } - return 0; -} - -/* Set/clear the status regsiter write protect bit in SR1. */ -static int generic_set_srp0(const struct flashctx *flash, int enable) -{ - uint8_t status, expected, check_mask; - struct wp_context *wp; - int num_entries; - - if (generic_range_table(flash, &wp, &num_entries)) - return -1; - - expected = do_read_status(flash); - msg_cdbg("%s: old status: 0x%02x\n", __func__, expected); - - if (enable) - expected |= 1 << wp->sr1.srp_pos; - else - expected &= ~(1 << wp->sr1.srp_pos); - - do_write_status(flash, expected); - - status = do_read_status(flash); - msg_cdbg("%s: new status: 0x%02x\n", __func__, status); - - check_mask = generic_get_status_check_mask(wp); - msg_cdbg("%s: check mask: 0x%02x\n", __func__, check_mask); - if ((status & check_mask) != (expected & check_mask)) { - msg_cerr("expected=0x%02x, but actual=0x%02x. check mask=0x%02x\n", - expected, status, check_mask); - return -1; - } - - return 0; -} - -static int generic_enable_writeprotect(const struct flashctx *flash, - enum wp_mode wp_mode) -{ - int ret; - - if (wp_mode != WP_MODE_HARDWARE) { - msg_cerr("%s(): unsupported write-protect mode\n", __func__); - return 1; - } - - ret = generic_set_srp0(flash, 1); - if (ret) - msg_cerr("%s(): error=%d.\n", __func__, ret); - - return ret; -} - -static int generic_disable_writeprotect(const struct flashctx *flash) -{ - int ret; - - ret = generic_set_srp0(flash, 0); - if (ret) - msg_cerr("%s(): error=%d.\n", __func__, ret); - - return ret; -} - -static int generic_list_ranges(const struct flashctx *flash) -{ - struct wp_context *wp; - struct wp_range_descriptor *r; - int i, num_entries; - - if (generic_range_table(flash, &wp, &num_entries)) - return -1; - - r = &wp->descrs[0]; - for (i = 0; i < num_entries; i++) { - msg_cinfo("start: 0x%06x, length: 0x%06x\n", - r->range.start, r->range.len); - r++; - } - - return 0; -} - -static int wp_context_status(const struct flashctx *flash) -{ - uint8_t sr1; - unsigned int start, len; - int ret = 0; - struct wp_context *wp; - int num_entries, wp_en; - - if (generic_range_table(flash, &wp, &num_entries)) - return -1; - - sr1 = do_read_status(flash); - wp_en = (sr1 >> wp->sr1.srp_pos) & 1; - - msg_cinfo("WP: status: 0x%04x\n", sr1); - msg_cinfo("WP: status.srp0: %x\n", wp_en); - /* FIXME: SRP1 is not really generic, but we probably should print - * it anyway to have consistent output. #legacycruft */ - msg_cinfo("WP: status.srp1: %x\n", 0); - msg_cinfo("WP: write protect is %s.\n", - wp_en ? "enabled" : "disabled"); - - msg_cinfo("WP: write protect range: "); - if (generic_status_to_range(flash, sr1, &start, &len)) { - msg_cinfo("(cannot resolve the range)\n"); - ret = -1; - } else { - msg_cinfo("start=0x%08x, len=0x%08x\n", start, len); - } - - return ret; -} - -struct wp wp_generic = { - .list_ranges = generic_list_ranges, - .set_range = generic_set_range, - .enable = generic_enable_writeprotect, - .disable = generic_disable_writeprotect, - .wp_status = wp_context_status, -}; diff --git a/writeprotect.h b/writeprotect.h index bded4c8..db7f3aa 100644 --- a/writeprotect.h +++ b/writeprotect.h @@ -16,36 +16,30 @@ */ #ifndef __WRITEPROTECT_H__ -#define __WRITEPROTECT_H__ 1 +#define __WRITEPROTECT_H__ enum wp_mode { - WP_MODE_UNKNOWN = -1, - WP_MODE_HARDWARE, /* hardware WP pin determines status */ - WP_MODE_POWER_CYCLE, /* WP active until power off/on cycle */ - WP_MODE_PERMANENT, /* status register permanently locked, - WP permanently enabled */ + WP_MODE_DISABLED, + WP_MODE_HARDWARE, + WP_MODE_POWER_CYCLE, + WP_MODE_PERMANENT, }; -struct wp { - int (*list_ranges)(const struct flashctx *flash); - int (*set_range)(const struct flashctx *flash, - unsigned int start, unsigned int len); - int (*enable)(const struct flashctx *flash, enum wp_mode mode); - int (*disable)(const struct flashctx *flash); - int (*wp_status)(const struct flashctx *flash); +struct wp_range { + uint32_t chip_len; + uint32_t start; + uint32_t len; }; -extern struct wp wp_generic; +bool wp_supported(struct flashctx *flash); -enum wp_mode get_wp_mode(const char *mode_str); +int wp_get_range(struct flashctx *flash, struct wp_range *range); +int wp_set_range(struct flashctx *flash, struct wp_range range); -/* - * Generic write-protect stuff - */ +int wp_get_mode(struct flashctx *flash, enum wp_mode *mode); +int wp_set_mode(struct flashctx *flash, enum wp_mode mode); -struct modifier_bits { - int sec; /* if 1, bp bits describe sectors */ - int tb; /* value of top/bottom select bit */ -}; +int wp_list_ranges(struct flashctx *flash); +int wp_print_status(struct flashctx *flash); -#endif /* !__WRITEPROTECT_H__ */ +#endif -- To view, visit https://review.coreboot.org/c/flashrom/+/58474 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I67e9b31f86465e5a8f7d3def637198671ee818a8 Gerrit-Change-Number: 58474 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: ich_descriptors_tool/Makefile: TARGET_OS is given as cmdline argument
by Nico Huber (Code Review) 20 Oct '21

20 Oct '21

Attention is currently required from: Thomas Heijligen. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58468 ) Change subject: ich_descriptors_tool/Makefile: TARGET_OS is given as cmdline argument ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Doesn't this break standalone builds? -- To view, visit https://review.coreboot.org/c/flashrom/+/58468 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I353c3de250167994a4aea1edfef57d839e900d78 Gerrit-Change-Number: 58468 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Wed, 20 Oct 2021 17:25:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add init-shutdown test for raiden_debug_spi
by Nico Huber (Code Review) 20 Oct '21

20 Oct '21

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57918 ) Change subject: tests: Add init-shutdown test for raiden_debug_spi ...................................................................... Patch Set 7: (1 comment) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/57918/comment/b6c41659_96366406 PS6, Line 57: ss > Specifically here, there were no warnings about raiden functions. But CONFIG_RAIDEN_DEBUG_SPI is enabled by default, maybe that's why? I expected warnings because these functions were neither declared static nor in a forward-declaration (e.g. in a header file). This is usually enabled by GCC's `-Wmissing-prototypes` option. It seems the `meson.build` just uses different warning options than the `Makefile`. That's not a good idea because reviewers are used to these warnings and probably won't check things that they assume the compiled checked. The `Makefile` also has a switch `WARNERROR` that defaults to `yes`. We could have something similar for the tests that may default to `no` if you prefer but would be enable by `test_build.sh` for the CI? > > And even more global question, about switching away from meson for tests, there is a lot to think about. But we do need to think about it, yes. I guess the problem is that the current `meson.build` was not meant for general use. Somebody added their use case but maintaining it beyond that was not the plan. Thomas is working to re-structure flashrom so we might be able to perform a cleaner switch to Meson in the future. If this will ever happen, it will take quite some time until then. If we want to use Meson for the tests, it might be better if somebody took over maintenance for it right now. -- To view, visit https://review.coreboot.org/c/flashrom/+/57918 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I880a8637ab02de179df9169c1898230bce4dc1c7 Gerrit-Change-Number: 57918 Gerrit-PatchSet: 7 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 20 Oct 2021 16:22:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: mark IS25LP064 as TEST_OK_PREW
by Simon Buhrow (Code Review) 20 Oct '21

20 Oct '21

Simon Buhrow has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/58472 ) Change subject: flashchips.c: mark IS25LP064 as TEST_OK_PREW ...................................................................... flashchips.c: mark IS25LP064 as TEST_OK_PREW Tested '-w', '-E' and '-r' successfully with my FT2232H programmer. Change-Id: I2197ce0be9db7c3d74b24c7445dc06238584ffea Signed-off-by: Simon Buhrow <simon.buhrow(a)posteo.de> --- M flashchips.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/72/58472/1 diff --git a/flashchips.c b/flashchips.c index 434c46d..848ebff 100644 --- a/flashchips.c +++ b/flashchips.c @@ -7311,7 +7311,7 @@ .page_size = 256, /* OTP: 1024B total; read 0x48; write 0x42 */ .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP, - .tested = TEST_UNTESTED, + .tested = TEST_OK_PREW, .probe = probe_spi_rdid, .probe_timing = TIMING_ZERO, .block_erasers = -- To view, visit https://review.coreboot.org/c/flashrom/+/58472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2197ce0be9db7c3d74b24c7445dc06238584ffea Gerrit-Change-Number: 58472 Gerrit-PatchSet: 1 Gerrit-Owner: Simon Buhrow Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: flashchips.c: big erase blocksize first
by Simon Buhrow (Code Review) 20 Oct '21

20 Oct '21

Attention is currently required from: Nico Huber. Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58467 ) Change subject: flashchips.c: big erase blocksize first ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Just a starting point for discussion. I used the following comands: time sudo flashrom -p ft2232_spi:type=2232H,port=A -E -VV and time sudo flashrom -p ft2232_spi:type=2232H,port=A -w write_64Mb_flash.bin -VV In the first case the order modification does save more than 2 minutes. In the second case it saved 2 seconds (the .bin file has only the first word different from a erased flash). Just let me know if this setup covers in a satisfying way the range of scenarios. If needed I can upload the flashrom output. -- To view, visit https://review.coreboot.org/c/flashrom/+/58467 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I862ce0b5f8912565e43c340578d8126aa2e6aa3b Gerrit-Change-Number: 58467 Gerrit-PatchSet: 1 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Wed, 20 Oct 2021 14:09:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors_tool/Makefile: TARGET_OS is given as cmdline argument
by Thomas Heijligen (Code Review) 20 Oct '21

20 Oct '21

Thomas Heijligen has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/58468 ) Change subject: ich_descriptors_tool/Makefile: TARGET_OS is given as cmdline argument ...................................................................... ich_descriptors_tool/Makefile: TARGET_OS is given as cmdline argument TARGET_OS is determined at the top level Makefile and pass to the ich_descriptors_tool Makefile. No need to determine TARGET_OS again. Change-Id: I353c3de250167994a4aea1edfef57d839e900d78 Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.de> --- M util/ich_descriptors_tool/Makefile 1 file changed, 0 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/68/58468/1 diff --git a/util/ich_descriptors_tool/Makefile b/util/ich_descriptors_tool/Makefile index 168220c..f668c53 100644 --- a/util/ich_descriptors_tool/Makefile +++ b/util/ich_descriptors_tool/Makefile @@ -4,8 +4,6 @@ # This Makefile works standalone, but it is usually called from the main # Makefile in the flashrom directory. -include ../../Makefile.include - PROGRAM=ich_descriptors_tool EXTRAINCDIRS = ../../ . DEPPATH = .dep @@ -34,8 +32,6 @@ FLASHROM_CFLAGS += -D__USE_MINGW_ANSI_STDIO=1 endif -override TARGET_OS := $(call c_macro_test, ../../Makefile.d/os_test.h) - ifeq ($(TARGET_OS), DOS) EXEC_SUFFIX := .exe # DJGPP has odd uint*_t definitions which cause lots of format string warnings. -- To view, visit https://review.coreboot.org/c/flashrom/+/58468 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I353c3de250167994a4aea1edfef57d839e900d78 Gerrit-Change-Number: 58468 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: flashchips.c: big erase blocksize first
by Simon Buhrow (Code Review) 20 Oct '21

20 Oct '21

Simon Buhrow has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/58467 ) Change subject: flashchips.c: big erase blocksize first ...................................................................... flashchips.c: big erase blocksize first Choosing the best fitting erase block size can safe a significant amount of runtime. Changing the order of entries of block_erasers in flashchips.c going from the bigger block size to lower block size does a better job. I am running flashrom inside a VirtualBox so the gained time is quite big. Setup: IS25LP064 flash + ft2232H programmer Saved time: > 2 min for single '-E' operation Change-Id: I862ce0b5f8912565e43c340578d8126aa2e6aa3b Signed-off-by: Simon Buhrow <simon.buhrow(a)posteo.de> --- M flashchips.c 1 file changed, 6 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/58467/1 diff --git a/flashchips.c b/flashchips.c index 434c46d..8212e19 100644 --- a/flashchips.c +++ b/flashchips.c @@ -7317,6 +7317,12 @@ .block_erasers = { { + .eraseblocks = { {8 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_c7, + }, { + .eraseblocks = { {8 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_60, + }, { .eraseblocks = { {4 * 1024, 2048} }, .block_erase = spi_block_erase_20, }, { @@ -7328,12 +7334,6 @@ }, { .eraseblocks = { {64 * 1024, 128} }, .block_erase = spi_block_erase_d8, - }, { - .eraseblocks = { {8 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_60, - }, { - .eraseblocks = { {8 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_c7, } }, .unlock = spi_disable_blockprotect, -- To view, visit https://review.coreboot.org/c/flashrom/+/58467 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I862ce0b5f8912565e43c340578d8126aa2e6aa3b Gerrit-Change-Number: 58467 Gerrit-PatchSet: 1 Gerrit-Owner: Simon Buhrow Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: Makefile: unify the use of filter
by Thomas Heijligen (Code Review) 20 Oct '21

20 Oct '21

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58451 ) Change subject: Makefile: unify the use of filter ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/58451/comment/1f326047_7dc4f10c PS1, Line 9: some cosmetics > Should have a verb, e.g. `fix some cosmetics`, `change some cosmetics`. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/58451 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6cd1e169b435cadb06423836cd9d64cdd2f51a94 Gerrit-Change-Number: 58451 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 20 Oct 2021 13:41:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Makefile: unify the use of filter
by Thomas Heijligen (Code Review) 20 Oct '21

20 Oct '21

Attention is currently required from: Thomas Heijligen. Hello build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/58451 to look at the new patch set (#2). Change subject: Makefile: unify the use of filter ...................................................................... Makefile: unify the use of filter Make a filter statement easier to read and fix some cosmetics. Change-Id: I6cd1e169b435cadb06423836cd9d64cdd2f51a94 Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.de> --- M Makefile 1 file changed, 4 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/51/58451/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/58451 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6cd1e169b435cadb06423836cd9d64cdd2f51a94 Gerrit-Change-Number: 58451 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2021