May 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: linux_mtd: move global state into programmer data field
by Nikolai Artemiev (Code Review) 08 May '21

08 May '21

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev, Anastasia Klimchuk. Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/53947 to look at the new patch set (#2). Change subject: linux_mtd: move global state into programmer data field ...................................................................... linux_mtd: move global state into programmer data field BUG=b:161951062 BRANCH=none TEST=builds, reading /dev/mtd0 on Oak succeeds Change-Id: I5ce6900e4892ed5687cfddb245dfe5461a3e2e84 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M linux_mtd.c 1 file changed, 65 insertions(+), 45 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/53947/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/53947 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5ce6900e4892ed5687cfddb245dfe5461a3e2e84 Gerrit-Change-Number: 53947 Gerrit-PatchSet: 2 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: buspirate_spi.c: Refactor singleton states into reentrant pattern
by Nico Huber (Code Review) 08 May '21

08 May '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52958 ) Change subject: buspirate_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 2: (2 comments) File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/52958/comment/cca037ab_c3e0c369 PS2, Line 167: struct buspirate_spi_data *buspirate_data = data; > I can probably create bp_commbuf as a local variable, alloc at the beginning and free at the end. Absolutely, yes. As DEFAULT_BUFSIZE is small, it could also live on the stack in most cases. > bp_commbufsize is different, I can see why it needs to be in a spi data in flashctx. AFAICT, it's only there to track the size of `bp_commbuf`. So they should share a common fate. I guess Angel is right, somebody tried to optimize things. Not in any way that would gain us something, though. malloc() and free() are not expensive, realloc() may be for bigger sizes. Compared to a single byte transferred over a serial line, it's still nothing. There's a third alternative. The size is eventually limited by what the v2 command supports. Somewhere below max_data_read/write is given. We could just allocate the maximum during init (256 + 5 + 2048 + 1). Define this as BUFFER_SIZE and let the command functions check that their parameters fit. Or a combination: use the stack during init, shutdown, command_v1 and only allocate the buffer if we use command_v2. https://review.coreboot.org/c/flashrom/+/52958/comment/da0c6345_aa3e5ecb PS2, Line 325: int bp_commbufsize = 0; We could declare it as follows /* 16 bytes data, 3 bytes control. */ unsigned char commbuf[16 + 3]; Then use `sizeof(commbuf)` instead of DEFAULT_BUFSIZE below. Something similar could be done to buspirate_shutdown(). All other users of the `bp_commbuf` use buspirate_commbuf_grow() and can use malloc()/free() instead. Hmm, maybe the v1 function should use the stack as well? -- To view, visit https://review.coreboot.org/c/flashrom/+/52958 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I418bbfff15fb126b042fbc9be09dbf59f4d243b8 Gerrit-Change-Number: 52958 Gerrit-PatchSet: 2 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> 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: Sat, 08 May 2021 10:50:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_mtd: move global state into programmer data field
by Angel Pons (Code Review) 08 May '21

08 May '21

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/53947 ) Change subject: linux_mtd: move global state into programmer data field ...................................................................... Patch Set 1: Code-Review+1 (2 comments) File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/53947/comment/9457ea37_002214bf PS1, Line 411: if missing a space between `if` and opening brace https://review.coreboot.org/c/flashrom/+/53947/comment/da4f6ed9_0b6d32a1 PS1, Line 417: free(data); I would like to put the `free(data);` call after the `linux_mtd_init_exit` label, but the handling of `param` would need to be refactored, which is out of the scope of this patch. -- To view, visit https://review.coreboot.org/c/flashrom/+/53947 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5ce6900e4892ed5687cfddb245dfe5461a3e2e84 Gerrit-Change-Number: 53947 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 08 May 2021 10:29:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: add support for W25Q32JW..IM
by Angel Pons (Code Review) 08 May '21

08 May '21

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/53946 ) Change subject: flashchips.c: add support for W25Q32JW..IM ...................................................................... Patch Set 1: (5 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/53946/comment/33c162d1_7538f38a PS1, Line 10: commit 1fc77dd1ee27a5d6e58a82c6ed6ed390a15372d7 For reference, this is CL:2160487 (No action needed, this comment just provides a direct link to the upstream change for convenience) File flashchips.c: https://review.coreboot.org/c/flashrom/+/53946/comment/eb28bb62_ca7cb4ce PS1, Line 17583: ..IM For consistency with W25Q64JW, I'd drop the trailing `..IM` suffix https://review.coreboot.org/c/flashrom/+/53946/comment/82b761eb_4887ecb9 PS1, Line 17589: FEATURE_WRSR_WREN Datasheet says that OTP is supported https://review.coreboot.org/c/flashrom/+/53946/comment/53d9e88b_0474b7ee PS1, Line 17611: }, Add: .printlock = spi_prettyprint_status_register_bp2_tb_bpl, https://review.coreboot.org/c/flashrom/+/53946/comment/c868e9aa_a551106e PS1, Line 17612: spi_disable_blockprotect spi_disable_blockprotect_bp2_srwd seems more accurate. Not perfect, but better. -- To view, visit https://review.coreboot.org/c/flashrom/+/53946 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7425e12658dd69c4ec8d3309dd591d09a935bb4d Gerrit-Change-Number: 53946 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sat, 08 May 2021 10:21:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: buspirate_spi.c: Refactor singleton states into reentrant pattern
by Angel Pons (Code Review) 08 May '21

08 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52958 ) Change subject: buspirate_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 2: (1 comment) File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/52958/comment/fb9bb438_d943e1bd PS2, Line 167: struct buspirate_spi_data *buspirate_data = data; > Thank you for this comment, because I started resolving it and so spent some more time staring at th […] bp_commbuf was global to minimize the number of times one needs to call malloc() or realloc(). There's a comment in buspirate_commbuf_grow() that gives a hint: /* Never shrink. realloc() calls are expensive. */ -- To view, visit https://review.coreboot.org/c/flashrom/+/52958 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I418bbfff15fb126b042fbc9be09dbf59f4d243b8 Gerrit-Change-Number: 52958 Gerrit-PatchSet: 2 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 08 May 2021 10:09:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_mtd: move global state into programmer data field
by Nikolai Artemiev (Code Review) 08 May '21

08 May '21

Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/53947 ) Change subject: linux_mtd: move global state into programmer data field ...................................................................... linux_mtd: move global state into programmer data field BUG=b:161951062 BRANCH=none TEST=builds, reading /dev/mtd0 on Oak succeeds Change-Id: I5ce6900e4892ed5687cfddb245dfe5461a3e2e84 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M linux_mtd.c 1 file changed, 65 insertions(+), 45 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/53947/1 diff --git a/linux_mtd.c b/linux_mtd.c index 22702e9..2785985 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -31,16 +31,16 @@ #define LINUX_DEV_ROOT "/dev" #define LINUX_MTD_SYSFS_ROOT "/sys/class/mtd" -static FILE *dev_fp = NULL; - -static int mtd_device_is_writeable; - -static int mtd_no_erase; - -/* Size info is presented in bytes in sysfs. */ -static unsigned long int mtd_total_size; -static unsigned long int mtd_numeraseregions; -static unsigned long int mtd_erasesize; /* only valid if numeraseregions is 0 */ +struct linux_mtd_data { + FILE *dev_fp; + int mtd_device_is_writeable; + int mtd_no_erase; + /* Size info is presented in bytes in sysfs. */ + unsigned long int mtd_total_size; + unsigned long int mtd_numeraseregions; + /* only valid if numeraseregions is 0 */ + unsigned long int mtd_erasesize; +}; /* read a string from a sysfs file and sanitize it */ static int read_sysfs_string(const char *sysfs_path, const char *filename, char *buf, int len) @@ -120,7 +120,7 @@ } /* returns 0 to indicate success, non-zero to indicate error */ -static int get_mtd_info(const char *sysfs_path) +static int get_mtd_info(const char *sysfs_path, struct linux_mtd_data *data) { unsigned long int tmp; char mtd_device_name[32]; @@ -130,10 +130,10 @@ return 1; if (tmp & MTD_WRITEABLE) { /* cache for later use by write function */ - mtd_device_is_writeable = 1; + data->mtd_device_is_writeable = 1; } if (tmp & MTD_NO_ERASE) { - mtd_no_erase = 1; + data->mtd_no_erase = 1; } /* Device name */ @@ -141,55 +141,59 @@ return 1; /* Total size */ - if (read_sysfs_int(sysfs_path, "size", &mtd_total_size)) + if (read_sysfs_int(sysfs_path, "size", &data->mtd_total_size)) return 1; - if (popcnt(mtd_total_size) != 1) { + if (popcnt(data->mtd_total_size) != 1) { msg_perr("MTD size is not a power of 2\n"); return 1; } /* Erase size */ - if (read_sysfs_int(sysfs_path, "erasesize", &mtd_erasesize)) + if (read_sysfs_int(sysfs_path, "erasesize", &data->mtd_erasesize)) return 1; - if (popcnt(mtd_erasesize) != 1) { + if (popcnt(data->mtd_erasesize) != 1) { msg_perr("MTD erase size is not a power of 2\n"); return 1; } /* Erase regions */ - if (read_sysfs_int(sysfs_path, "numeraseregions", &mtd_numeraseregions)) + if (read_sysfs_int(sysfs_path, "numeraseregions", &data->mtd_numeraseregions)) return 1; - if (mtd_numeraseregions != 0) { + if (data->mtd_numeraseregions != 0) { msg_perr("Non-uniform eraseblock size is unsupported.\n"); return 1; } msg_pdbg("%s: device_name: \"%s\", is_writeable: %d, " "numeraseregions: %lu, total_size: %lu, erasesize: %lu\n", - __func__, mtd_device_name, mtd_device_is_writeable, - mtd_numeraseregions, mtd_total_size, mtd_erasesize); + __func__, mtd_device_name, data->mtd_device_is_writeable, + data->mtd_numeraseregions, data->mtd_total_size, data->mtd_erasesize); return 0; } static int linux_mtd_probe(struct flashctx *flash) { - if (mtd_no_erase) + struct linux_mtd_data *data = flash->mst->opaque.data; + + if (data->mtd_no_erase) flash->chip->feature_bits |= FEATURE_NO_ERASE; flash->chip->tested = TEST_OK_PREW; - flash->chip->total_size = mtd_total_size / 1024; /* bytes -> kB */ - flash->chip->block_erasers[0].eraseblocks[0].size = mtd_erasesize; - flash->chip->block_erasers[0].eraseblocks[0].count = mtd_total_size / mtd_erasesize; + flash->chip->total_size = data->mtd_total_size / 1024; /* bytes -> kB */ + flash->chip->block_erasers[0].eraseblocks[0].size = data->mtd_erasesize; + flash->chip->block_erasers[0].eraseblocks[0].count = + data->mtd_total_size / data->mtd_erasesize; return 1; } static int linux_mtd_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { + struct linux_mtd_data *data = flash->mst->opaque.data; unsigned int eb_size = flash->chip->block_erasers[0].eraseblocks[0].size; unsigned int i; - if (fseek(dev_fp, start, SEEK_SET) != 0) { + if (fseek(data->dev_fp, start, SEEK_SET) != 0) { msg_perr("Cannot seek to 0x%06x: %s\n", start, strerror(errno)); return 1; } @@ -203,7 +207,7 @@ unsigned int step = eb_size - ((start + i) % eb_size); step = min(step, len - i); - if (fread(buf + i, step, 1, dev_fp) != 1) { + if (fread(buf + i, step, 1, data->dev_fp) != 1) { msg_perr("Cannot read 0x%06x bytes at 0x%06x: %s\n", step, start + i, strerror(errno)); return 1; @@ -219,13 +223,14 @@ static int linux_mtd_write(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) { + struct linux_mtd_data *data = flash->mst->opaque.data; unsigned int chunksize = flash->chip->block_erasers[0].eraseblocks[0].size; unsigned int i; - if (!mtd_device_is_writeable) + if (!data->mtd_device_is_writeable) return 1; - if (fseek(dev_fp, start, SEEK_SET) != 0) { + if (fseek(data->dev_fp, start, SEEK_SET) != 0) { msg_perr("Cannot seek to 0x%06x: %s\n", start, strerror(errno)); return 1; } @@ -240,12 +245,12 @@ unsigned int step = chunksize - ((start + i) % chunksize); step = min(step, len - i); - if (fwrite(buf + i, step, 1, dev_fp) != 1) { + if (fwrite(buf + i, step, 1, data->dev_fp) != 1) { msg_perr("Cannot write 0x%06x bytes at 0x%06x\n", step, start + i); return 1; } - if (fflush(dev_fp) == EOF) { + if (fflush(data->dev_fp) == EOF) { msg_perr("Failed to flush buffer: %s\n", strerror(errno)); return 1; } @@ -259,28 +264,29 @@ static int linux_mtd_erase(struct flashctx *flash, unsigned int start, unsigned int len) { + struct linux_mtd_data *data = flash->mst->opaque.data; uint32_t u; - if (mtd_no_erase) { + if (data->mtd_no_erase) { msg_perr("%s: device does not support erasing. Please file a " "bug report at flashrom(a)flashrom.org\n", __func__); return 1; } - if (mtd_numeraseregions != 0) { + if (data->mtd_numeraseregions != 0) { /* TODO: Support non-uniform eraseblock size using use MEMGETREGIONCOUNT/MEMGETREGIONINFO ioctls */ msg_perr("%s: mtd_numeraseregions must be 0\n", __func__); return 1; } - for (u = 0; u < len; u += mtd_erasesize) { + for (u = 0; u < len; u += data->mtd_erasesize) { struct erase_info_user erase_info = { .start = start + u, - .length = mtd_erasesize, + .length = data->mtd_erasesize, }; - if (ioctl(fileno(dev_fp), MEMERASE, &erase_info) == -1) { + if (ioctl(fileno(data->dev_fp), MEMERASE, &erase_info) == -1) { msg_perr("%s: ioctl: %s\n", __func__, strerror(errno)); return 1; } @@ -300,7 +306,7 @@ }; /* Returns 0 if setup is successful, non-zero to indicate error */ -static int linux_mtd_setup(int dev_num) +static int linux_mtd_setup(int dev_num, struct linux_mtd_data *data) { char sysfs_path[32]; int ret = 1; @@ -332,15 +338,15 @@ /* so far so good, get more info from other files in this dir */ if (snprintf(sysfs_path, sizeof(sysfs_path), "%s/mtd%d/", LINUX_MTD_SYSFS_ROOT, dev_num) < 0) goto linux_mtd_setup_exit; - if (get_mtd_info(sysfs_path)) + if (get_mtd_info(sysfs_path, data)) goto linux_mtd_setup_exit; /* open file stream and go! */ - if ((dev_fp = fopen(dev_path, "r+")) == NULL) { + if ((data->dev_fp = fopen(dev_path, "r+")) == NULL) { msg_perr("Cannot open file stream for %s\n", dev_path); goto linux_mtd_setup_exit; } - ret = setvbuf(dev_fp, NULL, _IONBF, 0); + ret = setvbuf(data->dev_fp, NULL, _IONBF, 0); if (ret) msg_pwarn("Failed to set MTD device to unbuffered: %d\n", ret); @@ -353,10 +359,12 @@ static int linux_mtd_shutdown(void *data) { - if (dev_fp != NULL) { - fclose(dev_fp); - dev_fp = NULL; + struct linux_mtd_data *mtd_data = data; + if (mtd_data->dev_fp != NULL) { + fclose(mtd_data->dev_fp); + mtd_data->dev_fp = NULL; } + free(data); return 0; } @@ -366,6 +374,7 @@ char *param; int dev_num = 0; int ret = 1; + struct linux_mtd_data *data = NULL; param = extract_programmer_param("dev"); if (param) { @@ -398,11 +407,22 @@ goto linux_mtd_init_exit; } - if (linux_mtd_setup(dev_num)) + data = calloc(1, sizeof(*data)); + if(!data) { + msg_perr("Unable to allocate memory for linux_mtd_data\n"); goto linux_mtd_init_exit; + } - if (register_shutdown(linux_mtd_shutdown, NULL)) + if (linux_mtd_setup(dev_num, data)) { + free(data); goto linux_mtd_init_exit; + } + + programmer_linux_mtd.data = data; + if (register_shutdown(linux_mtd_shutdown, (void *)data)) { + free(data); + goto linux_mtd_init_exit; + } register_opaque_master(&programmer_linux_mtd); -- To view, visit https://review.coreboot.org/c/flashrom/+/53947 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5ce6900e4892ed5687cfddb245dfe5461a3e2e84 Gerrit-Change-Number: 53947 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: flashchips.c: add support for W25Q32JW..IM
by Nikolai Artemiev (Code Review) 08 May '21

08 May '21

Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/53946 ) Change subject: flashchips.c: add support for W25Q32JW..IM ...................................................................... flashchips.c: add support for W25Q32JW..IM The chip was added to cros flashrom in `commit 1fc77dd1ee27a5d6e58a82c6ed6ed390a15372d7`. Quoting from the commit message: > We have varied the correct chip name is reported as well as > write and read 16MBytes of random data and verified the checksum's match. > Further, --wp-list appears to report the correct ranges. > > BUG=b:130199963 > BRANCH=none > TEST=Ran flashrom with a Dediprog SF100, RW random data and checksum matched. Change-Id: I7425e12658dd69c4ec8d3309dd591d09a935bb4d Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashchips.c M flashchips.h 2 files changed, 38 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/53946/1 diff --git a/flashchips.c b/flashchips.c index 6482f67..7f5d74b 100644 --- a/flashchips.c +++ b/flashchips.c @@ -17580,6 +17580,43 @@ { .vendor = "Winbond", + .name = "W25Q32JW..IM", + .bustype = BUS_SPI, + .manufacture_id = WINBOND_NEX_ID, + .model_id = WINBOND_NEX_W25Q32JW, + .total_size = 4096, + .page_size = 256, + .feature_bits = FEATURE_WRSR_WREN, + .tested = TEST_OK_PREW, + .probe = probe_spi_rdid, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 1024} }, + .block_erase = spi_block_erase_20, + }, { + .eraseblocks = { {32 * 1024, 128} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 64} }, + .block_erase = spi_block_erase_d8, + }, { + .eraseblocks = { {4 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_60, + }, { + .eraseblocks = { {4 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_c7, + } + }, + .unlock = spi_disable_blockprotect, + .write = spi_chip_write_256, + .read = spi_chip_read, + .voltage = {1700, 1950}, + }, + + { + .vendor = "Winbond", .name = "W25Q40.V", .bustype = BUS_SPI, .manufacture_id = WINBOND_NEX_ID, diff --git a/flashchips.h b/flashchips.h index b6b75c8..c01705f 100644 --- a/flashchips.h +++ b/flashchips.h @@ -977,6 +977,7 @@ #define WINBOND_NEX_W25Q256_W 0x6019 /* W25Q256JW */ #define WINBOND_NEX_W25Q128_V_M 0x7018 /* W25Q128JVSM */ #define WINBOND_NEX_W25Q256JV_M 0x7019 /* W25Q256JV_M (QE=0) */ +#define WINBOND_NEX_W25Q32JW 0x8016 #define WINBOND_NEX_W25Q64JW 0x8017 #define WINBOND_NEX_W25Q128_DTR 0x8018 /* W25Q128JW_DTR */ #define WINBOND_NEX_W25Q256_DTR 0x8019 /* W25Q256JW_DTR aka W25Q256256JW-IM */ -- To view, visit https://review.coreboot.org/c/flashrom/+/53946 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7425e12658dd69c4ec8d3309dd591d09a935bb4d Gerrit-Change-Number: 53946 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C
by Nikolai Artemiev (Code Review) 08 May '21

08 May '21

Attention is currently required from: Nico Huber, Angel Pons. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52883 ) Change subject: RFC: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Sorry, I'm not convinced. This looks like a workaround for something only downstream uses. […] Losing FEATURE_QPI definitely isn't ideal. I'm looking into whether the 'B' chips also support QPI; if they do does then I think merging would be ok, but please let me know if there are other issues. Having a way to ignore a list of chips would be useful too, though I think there is still benefit to merging chips that actually are identical, both for downstream and upstream users. -- To view, visit https://review.coreboot.org/c/flashrom/+/52883 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I688f2e15aef61afbec728a9a81094bee56d6fbfa Gerrit-Change-Number: 52883 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sat, 08 May 2021 05:07:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: buspirate_spi.c: Refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 08 May '21

08 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52958 ) Change subject: buspirate_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 2: (1 comment) File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/52958/comment/a426863c_94757414 PS2, Line 167: struct buspirate_spi_data *buspirate_data = data; > How about an […] Thank you for this comment, because I started resolving it and so spent some more time staring at the code. And I noticed something! It looks to me that bp_commbuf does not have a global meaning, its value is rewritten for every send operation (and for shutdown as well). Specifically what I mean is for these functions: 1) buspirate_spi_shutdown 2) buspirate_spi_send_command_v1 3) buspirate_spi_send_command_v2 3) buspirate_spi_init I can probably create bp_commbuf as a local variable, alloc at the beginning and free at the end. bp_commbufsize is different, I can see why it needs to be in a spi data in flashctx. buspirate_commbuf_grow and buspirate_sendrecv can take both as arguments. What I am worried about is, maybe I am missing something? Why bp_commbuf was initally a global variable, was it just for convenience or for a reason? What do you think? -- To view, visit https://review.coreboot.org/c/flashrom/+/52958 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I418bbfff15fb126b042fbc9be09dbf59f4d243b8 Gerrit-Change-Number: 52958 Gerrit-PatchSet: 2 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sat, 08 May 2021 02:48:16 +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]: cli_classic.c: Make -r/-w/-v argument optional
by Daniel Campello (Code Review) 08 May '21

08 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52362 ) Change subject: cli_classic.c: Make -r/-w/-v argument optional ...................................................................... Patch Set 18: (1 comment) Patchset: PS18: > Daniel, can we go the way of Nico's idea? Like I mention out of band a week or so ago I was never a […] See my comments on the other CL. For reference on *some* of the call sites: https://source.chromium.org/search?q=%5Cbflashrom%5Cb.*%5Cs(%5C-r%7C%5C-w%7… I believe a migration might be non trivial (if not infeasible) which might mean that we may have to maintain the 2 CLIs separate :( -- To view, visit https://review.coreboot.org/c/flashrom/+/52362 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I15384b92cb987a6ea1bb4369ef415488b9cf7f41 Gerrit-Change-Number: 52362 Gerrit-PatchSet: 18 Gerrit-Owner: Daniel Campello <campello(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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sat, 08 May 2021 01:48:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021