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

Change in ...flashrom[master]: freebsd_spi: Use malloc() instead of alloca()
by David Hendricks (Code Review) 10 Oct '21

10 Oct '21

David Hendricks has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/30765 Change subject: freebsd_spi: Use malloc() instead of alloca() ...................................................................... freebsd_spi: Use malloc() instead of alloca() Change-Id: I03ff2bd1e20bea014333945771b560f17387ebd6 Signed-off-by: David Hendricks <david.hendricks(a)gmail.com> --- M freebsd_spi.c 1 file changed, 7 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/65/30765/1 diff --git a/freebsd_spi.c b/freebsd_spi.c index 215be20..7514912 100644 --- a/freebsd_spi.c +++ b/freebsd_spi.c @@ -145,7 +145,11 @@ /* FreeBSD uses a single buffer for rx and tx. Allocate a temporary one to avoid overwriting anything. */ size_t tmpcnt = readcnt + writecnt; - unsigned char *tmpbuf = alloca(tmpcnt); + unsigned char *tmpbuf = malloc(tmpcnt); + if (!tmpbuf) { + msg_perr("Out of memory!\n"); + return -1; + } bzero(tmpbuf, tmpcnt); memcpy(tmpbuf, txbuf, writecnt); @@ -164,12 +168,14 @@ if (ioctl(fd, SPIGENIOC_TRANSFER, &msg) == -1) { msg_cerr("%s: ioctl: %s\n", __func__, strerror(errno)); + free(tmpbuf); return -1; } if (rxbuf) memcpy(rxbuf, tmpbuf + writecnt, readcnt); + free(tmpbuf); return 0; } -- To view, visit https://review.coreboot.org/c/flashrom/+/30765 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I03ff2bd1e20bea014333945771b560f17387ebd6 Gerrit-Change-Number: 30765 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: newchange

1 1

Change in ...flashrom[master]: Add freebsd_spi programmer
by David Hendricks (Code Review) 10 Oct '21

10 Oct '21

David Hendricks has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/30764 Change subject: Add freebsd_spi programmer ...................................................................... Add freebsd_spi programmer Based on linux_spi, using FreeBSD's spigen(4) interface. Change-Id: I4e1689416fbb309df94807f51635bc1f4b53e0c8 Signed-off-by: Greg V <greg(a)unrelenting.technology> --- M Makefile M flashrom.8.tmpl M flashrom.c A freebsd_spi.c M programmer.h 5 files changed, 246 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/30764/1 diff --git a/Makefile b/Makefile index 1ff578c..7e709dc 100644 --- a/Makefile +++ b/Makefile @@ -379,6 +379,14 @@ endif endif +ifneq ($(TARGET_OS), FreeBSD) +ifeq ($(CONFIG_FREEBSD_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_FREEBSD_SPI=yes +else +override CONFIG_FREEBSD_SPI = no +endif +endif + ############################################################################### # General architecture-specific settings. # Like above for the OS, below we verify user-supplied options depending on the target architecture. @@ -647,6 +655,9 @@ CONFIG_LINUX_MTD ?= yes CONFIG_LINUX_SPI ?= yes +# Enable FreeBSD spigen interface by default. We disable them on non-FreeBSD targets. +CONFIG_FREEBSD_SPI ?= yes + # Always enable ITE IT8212F PATA controllers for now. CONFIG_IT8212 ?= yes @@ -945,6 +956,11 @@ PROGRAMMER_OBJS += linux_spi.o endif +ifeq ($(CONFIG_FREEBSD_SPI), yes) +FEATURE_CFLAGS += -D'CONFIG_FREEBSD_SPI=1' +PROGRAMMER_OBJS += freebsd_spi.o +endif + ifeq ($(CONFIG_MSTARDDC_SPI), yes) # This is a totally ugly hack. FEATURE_CFLAGS += $(call debug_shell,grep -q "LINUX_I2C_SUPPORT := yes" .features && printf "%s" "-D'CONFIG_MSTARDDC_SPI=1'") diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index c557af7..092f070 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -319,6 +319,8 @@ .sp .BR "* linux_spi" " (for SPI flash ROMs accessible via /dev/spidevX.Y on Linux)" .sp +.BR "* freebsd_spi" " (for SPI flash ROMs accessible via /dev/spigenX.Y on FreeBSD)" +.sp .BR "* usbblaster_spi" " (for SPI flash ROMs attached to an Altera USB-Blaster compatible cable)" .sp .BR "* nicintel_eeprom" " (for SPI EEPROMs on Intel Gigabit network cards)" @@ -1065,6 +1067,25 @@ .sp Please note that the linux_spi driver only works on Linux. .SS +.BR "freebsd_spi " programmer +.IP +You have to specify the SPI controller to use with the +.sp +.B " flashrom \-p freebsd_spi:dev=/dev/spigenX.Y" +.sp +syntax where +.B /dev/spigenX.Y +is the FreeBSD device node for your SPI controller. +.sp +In case the device supports it, you can set the SPI clock frequency with the optional +.B spispeed +parameter. The frequency is parsed as kilohertz. +Example that sets the frequency to 8 MHz: +.sp +.B " flashrom \-p freebsd_spi:dev=/dev/spigenX.Y,spispeed=8000" +.sp +Please note that the freebsd_spi driver only works on FreeBSD. +.SS .BR "mstarddc_spi " programmer .IP The Display Data Channel (DDC) is an I2C bus present on VGA and DVI connectors, that allows exchanging diff --git a/flashrom.c b/flashrom.c index 59a7531..624bcfb 100644 --- a/flashrom.c +++ b/flashrom.c @@ -377,6 +377,18 @@ }, #endif +#if CONFIG_FREEBSD_SPI == 1 + { + .name = "freebsd_spi", + .type = OTHER, + .devs.note = "Device files /dev/spigen*.*\n", + .init = freebsd_spi_init, + .map_flash_region = fallback_map, + .unmap_flash_region = fallback_unmap, + .delay = internal_delay, + }, +#endif + #if CONFIG_USBBLASTER_SPI == 1 { .name = "usbblaster_spi", diff --git a/freebsd_spi.c b/freebsd_spi.c new file mode 100644 index 0000000..215be20 --- /dev/null +++ b/freebsd_spi.c @@ -0,0 +1,186 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2011 Sven Schnelle <svens(a)stackframe.org> + * Copyright (C) 2018 Greg V <greg(a)unrelenting.technology> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#if CONFIG_FREEBSD_SPI == 1 + +#include <stdio.h> +#include <string.h> +#include <stdlib.h> +#include <fcntl.h> +#include <errno.h> +#include <ctype.h> +#include <unistd.h> +#include <sys/ioctl.h> +#include <sys/types.h> +#include <sys/ioccom.h> +#include <sys/spigenio.h> +#include "flash.h" +#include "chipdrivers.h" +#include "programmer.h" +#include "spi.h" + +/* Tested on: + * Xunlong Orange Pi PC (Allwinner H3) */ + +/* Same as in spi(8) */ +#define DEFAULT_BUFFER_SIZE 8192 + +static int fd = -1; + +static int freebsd_spi_shutdown(void *data); +static int freebsd_spi_send_command(struct flashctx *flash, unsigned int writecnt, + unsigned int readcnt, + const unsigned char *txbuf, + unsigned char *rxbuf); +static int freebsd_spi_read(struct flashctx *flash, uint8_t *buf, + unsigned int start, unsigned int len); +static int freebsd_spi_write_256(struct flashctx *flash, const uint8_t *buf, + unsigned int start, unsigned int len); + +static const struct spi_master spi_master_freebsd = { + .type = SPI_CONTROLLER_FREEBSD, + .features = SPI_MASTER_4BA, + .max_data_read = MAX_DATA_UNSPECIFIED, /* TODO? */ + .max_data_write = MAX_DATA_UNSPECIFIED, /* TODO? */ + .command = freebsd_spi_send_command, + .multicommand = default_spi_send_multicommand, + .read = freebsd_spi_read, + .write_256 = freebsd_spi_write_256, + .write_aai = default_spi_write_aai, +}; + +int freebsd_spi_init(void) +{ + char *p, *endp, *dev; + uint32_t speed_hz = 0; + /* FIXME: make the following configurable by CLI options. */ + /* SPI mode 0 (beware this also includes: MSB first, CS active low and others */ + const uint8_t mode = 0; + + p = extract_programmer_param("spispeed"); + if (p && strlen(p)) { + speed_hz = (uint32_t)strtoul(p, &endp, 10) * 1000; + if (p == endp) { + msg_perr("%s: invalid clock: %s kHz\n", __func__, p); + free(p); + return 1; + } + } + free(p); + + dev = extract_programmer_param("dev"); + if (!dev || !strlen(dev)) { + msg_perr("No SPI device given. Use flashrom -p " + "freebsd_spi:dev=/dev/spigenX.Y\n"); + free(dev); + return 1; + } + + msg_pdbg("Using device %s\n", dev); + if ((fd = open(dev, O_RDWR)) == -1) { + msg_perr("%s: failed to open %s: %s\n", __func__, + dev, strerror(errno)); + free(dev); + return 1; + } + free(dev); + + if (register_shutdown(freebsd_spi_shutdown, NULL)) + return 1; + /* We rely on the shutdown function for cleanup from here on. */ + + if (speed_hz > 0) { + if (ioctl(fd, SPIGENIOC_SET_CLOCK_SPEED, &speed_hz) == -1) { + msg_perr("%s: failed to set speed to %d Hz: %s\n", + __func__, speed_hz, strerror(errno)); + return 1; + } + + msg_pdbg("Using %d kHz clock\n", speed_hz/1000); + } + + if (ioctl(fd, SPIGENIOC_SET_SPI_MODE, &mode) == -1) { + msg_perr("%s: failed to set SPI mode to 0x%02x: %s\n", + __func__, mode, strerror(errno)); + return 1; + } + + register_spi_master(&spi_master_freebsd); + return 0; +} + +static int freebsd_spi_shutdown(void *data) +{ + if (fd != -1) { + close(fd); + fd = -1; + } + return 0; +} + +static int freebsd_spi_send_command(struct flashctx *flash, unsigned int writecnt, + unsigned int readcnt, + const unsigned char *txbuf, + unsigned char *rxbuf) +{ + if (fd == -1) + return -1; + /* The implementation currently does not support requests that + don't start with sending a command. */ + if (writecnt == 0) + return SPI_INVALID_LENGTH; + + /* FreeBSD uses a single buffer for rx and tx. Allocate a temporary one to avoid overwriting anything. */ + size_t tmpcnt = readcnt + writecnt; + unsigned char *tmpbuf = alloca(tmpcnt); + + bzero(tmpbuf, tmpcnt); + memcpy(tmpbuf, txbuf, writecnt); + + /* Command/data separation is pretty useless, spi(8) only uses the command. */ + struct spigen_transfer msg = { + .st_command = { + .iov_base = (void*)tmpbuf, + .iov_len = tmpcnt, + }, + .st_data = { + .iov_base = NULL, + .iov_len = 0, + }, + }; + + if (ioctl(fd, SPIGENIOC_TRANSFER, &msg) == -1) { + msg_cerr("%s: ioctl: %s\n", __func__, strerror(errno)); + return -1; + } + + if (rxbuf) + memcpy(rxbuf, tmpbuf + writecnt, readcnt); + + return 0; +} + +static int freebsd_spi_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) +{ + return spi_read_chunked(flash, buf, start, len, DEFAULT_BUFFER_SIZE); +} + +static int freebsd_spi_write_256(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) +{ + return spi_write_chunked(flash, buf, start, len, DEFAULT_BUFFER_SIZE); +} + +#endif // CONFIG_FREEBSD_SPI == 1 diff --git a/programmer.h b/programmer.h index 311992a..30687ed 100644 --- a/programmer.h +++ b/programmer.h @@ -103,6 +103,9 @@ #if CONFIG_LINUX_SPI == 1 PROGRAMMER_LINUX_SPI, #endif +#if CONFIG_FREEBSD_SPI == 1 + PROGRAMMER_FREEBSD_SPI, +#endif #if CONFIG_USBBLASTER_SPI == 1 PROGRAMMER_USBBLASTER_SPI, #endif @@ -548,6 +551,11 @@ int linux_spi_init(void); #endif +/* freebsd_spi.c */ +#if CONFIG_FREEBSD_SPI == 1 +int freebsd_spi_init(void); +#endif + /* dediprog.c */ #if CONFIG_DEDIPROG == 1 int dediprog_init(void); @@ -623,6 +631,9 @@ #if CONFIG_LINUX_SPI == 1 SPI_CONTROLLER_LINUX, #endif +#if CONFIG_FREEBSD_SPI == 1 + SPI_CONTROLLER_FREEBSD, +#endif #if CONFIG_SERPROG == 1 SPI_CONTROLLER_SERPROG, #endif -- To view, visit https://review.coreboot.org/c/flashrom/+/30764 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4e1689416fbb309df94807f51635bc1f4b53e0c8 Gerrit-Change-Number: 30764 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: newchange

1 2

Change in flashrom[master]: util/ich_descriptors_tool: Use GNU-style printf in MinGW
by David Hendricks (Code Review) 01 Oct '21

01 Oct '21

David Hendricks has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43052 ) Change subject: util/ich_descriptors_tool: Use GNU-style printf in MinGW ...................................................................... util/ich_descriptors_tool: Use GNU-style printf in MinGW This allows MinGW targets to use certain printf formatting identifiers such as "%v" by adding -D__USE_MINGW_ANSI_STDIO=1 to the CFLAGS. This is also done in flashrom's top-level Makefile. Reported on https://github.com/flashrom/flashrom/issues/149 Change-Id: I644be8b5b607cc77b4be2121c443f0d41d8da687 Signed-off-by: David Hendricks <david.hendricks(a)gmail.com> --- M util/ich_descriptors_tool/Makefile 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/52/43052/1 diff --git a/util/ich_descriptors_tool/Makefile b/util/ich_descriptors_tool/Makefile index c32e30b..8962431 100644 --- a/util/ich_descriptors_tool/Makefile +++ b/util/ich_descriptors_tool/Makefile @@ -41,6 +41,10 @@ CFLAGS += -Wno-format endif +ifeq ($(TARGET_OS), MinGW) +CFLAGS += -D__USE_MINGW_ANSI_STDIO=1 +endif + ifeq ($(WARNERROR), yes) CFLAGS += -Werror endif -- To view, visit https://review.coreboot.org/c/flashrom/+/43052 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I644be8b5b607cc77b4be2121c443f0d41d8da687 Gerrit-Change-Number: 43052 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: newchange

4 5

Change in flashrom[master]: ft2232_spi.c: change the chunksize to 280
by Simon Buhrow (Code Review) 27 Sep '21

27 Sep '21

Simon Buhrow has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43282 ) Change subject: ft2232_spi.c: change the chunksize to 280 ...................................................................... ft2232_spi.c: change the chunksize to 280 Signed-off-by: Simon Buhrow <simon.buhrow(a)posteo.de> Change-Id: I5e096937834fc8fc8623c8c59ade529de081b72a --- M ft2232_spi.c 1 file changed, 12 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/43282/1 diff --git a/ft2232_spi.c b/ft2232_spi.c index 0799af4..0a8d631 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -398,7 +398,18 @@ msg_perr("Unable to set latency timer (%s).\n", ftdi_get_error_string(ftdic)); } - if (ftdi_write_data_set_chunksize(ftdic, 270)) { + if (ftdi_write_data_set_chunksize(ftdic, 280)) { + /* + * 280 bytes = + * + 9 B (CMD) + * + 1 B (WREN) + * + 9 B (CMD) + * + 1 B (op) + * + 4 B (addr) + * + 256 B (page data) + * + * With op: PageProgram or Erase; CMD: FTDI-Chip commands + */ msg_perr("Unable to set chunk size (%s).\n", ftdi_get_error_string(ftdic)); } -- To view, visit https://review.coreboot.org/c/flashrom/+/43282 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5e096937834fc8fc8623c8c59ade529de081b72a Gerrit-Change-Number: 43282 Gerrit-PatchSet: 1 Gerrit-Owner: Simon Buhrow Gerrit-MessageType: newchange

4 17

Change in flashrom[master]: flashrom: Return early from map_flash for >16MiB on x86
by Nico Huber (Code Review) 31 Jul '21

31 Jul '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Edward O'Callaghan. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56721 ) Change subject: flashrom: Return early from map_flash for >16MiB on x86 ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: IIRC, the call to map_flash() is spurious for most chips (all SPI maybe?), so I would prefer to avoid the call instead. -- To view, visit https://review.coreboot.org/c/flashrom/+/56721 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I60244b6970118dbdfdbb5b8424fc0a8acd9def2e Gerrit-Change-Number: 56721 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sat, 31 Jul 2021 21:51:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom: Return early from map_flash for >16MiB on x86
by Tim Wawrzynczak (Code Review) 30 Jul '21

30 Jul '21

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/56721 ) Change subject: flashrom: Return early from map_flash for >16MiB on x86 ...................................................................... flashrom: Return early from map_flash for >16MiB on x86 In the case of a flash chip bigger than 16 MiB, x86 chipsets are only able to map the topmost 16 MiB of the chip into memory. To access the rest, flashrom can use hardware sequencing, therefore return early from `map_flash()` in this situation; this avoids messages like the following in Linux kernel logs: [ 57.715654] x86/PAT: flashrom:2805 conflicting memory types fe0000... [ 57.726270] x86/PAT: memtype_reserve failed [mem 0xfe000000-0xffff... BUG=b:185021901 TEST=On a system with a W25Q256.V (32MB), flashrom logs show: $ flashrom -p host -r bios.bin ... Found Winbond flash chip "W25Q256.V" (32768 kB, Programmer-specific) ... Chipset unable to map >16 MiB of flash below 0xffffffff, falling back... Reading flash... done. SUCCESS Signed-off-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Change-Id: I60244b6970118dbdfdbb5b8424fc0a8acd9def2e --- M flashrom.c 1 file changed, 11 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/56721/1 diff --git a/flashrom.c b/flashrom.c index 3987bb9..dea96b7 100644 --- a/flashrom.c +++ b/flashrom.c @@ -697,6 +697,17 @@ return 0; const chipsize_t size = flash->chip->total_size * 1024; + +#if IS_X86 + /* No known x86 chipset maps more than 16 MiB of the flash into memory + * below 0xfffffff, therefore return early here. */ + if (size > 16 * MiB) { + msg_cinfo("Chipset unable to map >16 MiB of flash below 0xffffffff," + "falling back to hardware sequencing.\n"); + return 1; + } +#endif + uintptr_t base = flashbase ? flashbase : (0xffffffff - size + 1); void *addr = programmer_map_flash_region(flash->chip->name, base, size); if (addr == ERROR_PTR) { -- To view, visit https://review.coreboot.org/c/flashrom/+/56721 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I60244b6970118dbdfdbb5b8424fc0a8acd9def2e Gerrit-Change-Number: 56721 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: programmer.h: Minor tidy up
by Nico Huber (Code Review) 30 Jul '21

30 Jul '21

Attention is currently required from: Edward O'Callaghan. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56466 ) Change subject: programmer.h: Minor tidy up ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Very likely not in most cases. […] Hmmm, looking at this from another angle: I think we should just split the file. That would make guarding implicit, right? e.g. if you some code doesn't need USB stuff, it wouldn't include the usb header file. If all these declarations can be clearly grouped, then I see no reason to have them in a monolithic header file. WDYT? -- To view, visit https://review.coreboot.org/c/flashrom/+/56466 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7ceda32b66d17318d42b94a1fab621a9a926fa77 Gerrit-Change-Number: 56466 Gerrit-PatchSet: 2 Gerrit-Owner: 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-Comment-Date: Fri, 30 Jul 2021 14:29:01 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add tests to erase a chip
by Anastasia Klimchuk (Code Review) 30 Jul '21

30 Jul '21

Attention is currently required from: Nico Huber, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56501 ) Change subject: tests: Add tests to erase a chip ...................................................................... Patch Set 6: (1 comment) Patchset: PS2: > I looked again, and realised I want to improve my solution a bit. […] Some more explanations (sorry for spam :D) My strong preference to have only one flashrom_test_dep in root meson.build , because if there are two dependencies this creates a "choice" and a question "which one should I choose for my new test? which one is the right one?" and a chance to pick wrong one. There should not be a choice in this situation, only one flashrom_test_dep with all compile args set up for test context. A side-effect is that all test_outx and test_inx need to be wrapped for all tests executables, even if the test itself is not using them. Those wraps can be empty, but need to be present. Potentially later this can be improved, maybe wraps extracted into separate .c file. There are lots of wraps already, and there will be more (because there will be more tests!) -- To view, visit https://review.coreboot.org/c/flashrom/+/56501 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898 Gerrit-Change-Number: 56501 Gerrit-PatchSet: 6 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 30 Jul 2021 06:12:42 +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]: tests: Add tests to erase a chip
by Anastasia Klimchuk (Code Review) 30 Jul '21

30 Jul '21

Attention is currently required from: Nico Huber, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56501 ) Change subject: tests: Add tests to erase a chip ...................................................................... Patch Set 6: (1 comment) Patchset: PS2: > Yes, I remember that when function is wrapped, the real one is meant to be available as __real_funct […] I looked again, and realised I want to improve my solution a bit. I don't want any changes in root meson.build (reverted them), so now all changes are contained within tests/ Still two test executables, but both of them using the same flashrom_test_dep from root meson.build. The thing which is not ideal: wraps for inx and outx are repeated in wrap_send_tests.c (they can do nothing, but need to be present). I am thinking how to make this better... but maybe this can be done later? (if everyone agrees of course). Next thing I am going to split the patch into two. -- To view, visit https://review.coreboot.org/c/flashrom/+/56501 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898 Gerrit-Change-Number: 56501 Gerrit-PatchSet: 6 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 30 Jul 2021 05:40:10 +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]: tests: Add tests to erase a chip
by Anastasia Klimchuk (Code Review) 30 Jul '21

30 Jul '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/56501 to look at the new patch set (#6). Change subject: tests: Add tests to erase a chip ...................................................................... tests: Add tests to erase a chip Two tests cover the code which performs do_erase operation. First one works with fake chip and dummy programmer. Fake chip has all operations defined, and a buffer to emulate chip memory. Second one uses the chip which is closer to the real one, because read/write/unlock/erase operations are real. The tests takes the advantage of dummyflasher's capability of emulating a W25Q128.V chip. BUG=b:181803212 TEST=builds and ninja test Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- A tests/chip.c M tests/meson.build M tests/tests.c M tests/tests.h A tests/wrap_send_tests.c A tests/wrap_send_tests.h 6 files changed, 365 insertions(+), 34 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/01/56501/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/56501 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898 Gerrit-Change-Number: 56501 Gerrit-PatchSet: 6 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit July 2021