- flashrom-gerrit - mail.coreboot.org

[XS] Change in flashrom[main]: serprog: clean up documentation
by Anastasia Klimchuk (Code Review) 01 Mar '24

01 Mar '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/80499?usp=email ) Change subject: serprog: clean up documentation ...................................................................... serprog: clean up documentation * serprog.h doesn't exist, so refer to .c source instead * in the doc, no other command has S_CMD_ prefix either Change-Id: Ic83e7fd80840f2db0b006935a964721da0388068 Signed-off-by: Riku Viitanen <riku.viitanen(a)protonmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/80499 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M Documentation/serprog-protocol.txt 1 file changed, 2 insertions(+), 2 deletions(-) Approvals: Anastasia Klimchuk: Looks good to me, approved build bot (Jenkins): Verified diff --git a/Documentation/serprog-protocol.txt b/Documentation/serprog-protocol.txt index d58b8e9..3d4aa1a 100644 --- a/Documentation/serprog-protocol.txt +++ b/Documentation/serprog-protocol.txt @@ -84,7 +84,7 @@ lower than the one requested. If there is no lower frequency available the lowest possible should be used. The value chosen is sent back in the reply with an ACK. - 0x15 (S_CMD_S_PIN_STATE): + 0x15 (S_PIN_STATE): Sets the state of the pin drivers connected to the flash chip. Disabling them allows other devices (e.g. a mainboard's chipset) to access the chip. This way the serprog controller can remain attached to the flash chip even when the board is running. The user is responsible to @@ -103,4 +103,4 @@ In addition, support for these commands is recommended: S_CMD_Q_PGMNAME, S_CMD_Q_BUSTYPE, S_CMD_Q_CHIPSIZE (if parallel). -See also serprog.h. +See also serprog.c -- To view, visit https://review.coreboot.org/c/flashrom/+/80499?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic83e7fd80840f2db0b006935a964721da0388068 Gerrit-Change-Number: 80499 Gerrit-PatchSet: 4 Gerrit-Owner: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[main]: serprog: Add support for multiple SPI chip selects
by Anastasia Klimchuk (Code Review) 01 Mar '24

01 Mar '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/80498?usp=email ) Change subject: serprog: Add support for multiple SPI chip selects ...................................................................... serprog: Add support for multiple SPI chip selects Tested with an EliteBook 8560w, Pi Pico, and my serprog firmware: https://codeberg.org/Riku_V/pico-serprog/ As seen on Flashprog: https://review.sourcearcade.org/c/flashprog/+/51 Change-Id: If8052bc6f5c314dcc493bc083bb8270723efaae7 Signed-off-by: Riku Viitanen <riku.viitanen(a)protonmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/80498 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M Documentation/serprog-protocol.txt M doc/classic_cli_manpage.rst M serprog.c 3 files changed, 39 insertions(+), 1 deletion(-) Approvals: Anastasia Klimchuk: Looks good to me, approved build bot (Jenkins): Verified diff --git a/Documentation/serprog-protocol.txt b/Documentation/serprog-protocol.txt index 6b7e7e3..d58b8e9 100644 --- a/Documentation/serprog-protocol.txt +++ b/Documentation/serprog-protocol.txt @@ -35,6 +35,7 @@ + slen bytes of data 0x14 Set SPI clock frequency in Hz 32-bit requested frequency ACK + 32-bit set frequency / NAK 0x15 Toggle flash chip pin drivers 8-bit (0 disable, else enable) ACK / NAK +0x16 Set SPI Chip Select 8-bit ACK / NAK 0x?? unimplemented command - invalid. @@ -89,6 +90,9 @@ remain attached to the flash chip even when the board is running. The user is responsible to NOT connect VCC and other permanently externally driven signals to the programmer as needed. If the value is 0, then the drivers should be disabled, otherwise they should be enabled. + 0x16 (S_SPI_CS): + Set which SPI Chip Select pin to use. This operation is immediate, + meaning it doesn't use the operation buffer. About mandatory commands: The only truly mandatory commands for any device are 0x00, 0x01, 0x02 and 0x10, but one can't really do anything with these commands. diff --git a/doc/classic_cli_manpage.rst b/doc/classic_cli_manpage.rst index 0cd45ca..c46edf6 100644 --- a/doc/classic_cli_manpage.rst +++ b/doc/classic_cli_manpage.rst @@ -741,6 +741,14 @@ flashrom -p serprog:dev=/dev/device:baud,spispeed=2M +Optional ``cs`` parameter can be used to switch which chip select number is used. This allows connecting multiple +chips at once and selecting which one to flash by software means (rather than rewiring):: + + flashrom -p serprog:dev=/dev/device:baud,cs=0 + +The particular programmer implementation needs to support this feature, for it to work. If the requested chip +select isn't available, flashrom will fail safely. + More information about serprog is available in **serprog-protocol.txt** in the source distribution. diff --git a/serprog.c b/serprog.c index b51f0cf..c2965c5 100644 --- a/serprog.c +++ b/serprog.c @@ -3,6 +3,7 @@ * * Copyright (C) 2009, 2011 Urja Rannikko <urjaman(a)gmail.com> * Copyright (C) 2009 Carl-Daniel Hailfinger + * Copyright (C) 2024 Riku Viitanen <riku.viitanen(a)protonmail.com> * * 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 @@ -63,6 +64,7 @@ #define S_CMD_O_SPIOP 0x13 /* Perform SPI operation. */ #define S_CMD_S_SPI_FREQ 0x14 /* Set SPI clock frequency */ #define S_CMD_S_PIN_STATE 0x15 /* Enable/disable output drivers */ +#define S_CMD_S_SPI_CS 0x16 /* Set SPI chip select to use */ #define MSGHEADER "serprog: " @@ -742,7 +744,7 @@ /* Check for the minimum operational set of commands. */ if (serprog_buses_supported & BUS_SPI) { uint8_t bt = BUS_SPI; - char *spispeed; + char *spispeed, *cs; if (sp_check_commandavail(S_CMD_O_SPIOP) == 0) { msg_perr("Error: SPI operation not supported while the " "bustype is SPI\n"); @@ -822,6 +824,30 @@ } } free(spispeed); + cs = extract_programmer_param_str(cfg, "cs"); + if (cs && strlen(cs)) { + char *endptr = NULL; + errno = 0; + unsigned long cs_num = strtoul(cs, &endptr, 0); + if (errno || *endptr || cs_num > 255) { + msg_perr("Error: Invalid chip select requested! " + "Only 0-255 are valid.\n"); + free(cs); + goto init_err_cleanup_exit; + } + free(cs); + if (!sp_check_commandavail(S_CMD_S_SPI_CS)) { + msg_perr("Error: Setting SPI chip select is not supported!\n"); + goto init_err_cleanup_exit; + } + msg_pdbg(MSGHEADER "Requested to use chip select %lu.\n", cs_num); + uint8_t cs_num8 = cs_num; + if (sp_docommand(S_CMD_S_SPI_CS, 1, &cs_num8, 0, NULL)) { + msg_perr("Error: Chip select %u not supported " + "by programmer!\n", cs_num8); + goto init_err_cleanup_exit; + } + } bt = serprog_buses_supported; if (sp_docommand(S_CMD_S_BUSTYPE, 1, &bt, 0, NULL)) goto init_err_cleanup_exit; -- To view, visit https://review.coreboot.org/c/flashrom/+/80498?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If8052bc6f5c314dcc493bc083bb8270723efaae7 Gerrit-Change-Number: 80498 Gerrit-PatchSet: 4 Gerrit-Owner: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[XS] Change in flashrom[main]: udelay: Lower the sleep vs delay threshold
by Brian Norris (Code Review) 01 Mar '24

01 Mar '24

Brian Norris has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/80808?usp=email ) Change subject: udelay: Lower the sleep vs delay threshold ...................................................................... udelay: Lower the sleep vs delay threshold By default, we busy-loop (a.k.a., "delay") for most delay values, and only allow sleeping for large delays. But busy-looping is expensive, as it wastes CPU cycles. In a simple program that runs a bunch of samples of [1] over 1000 samples, I find that for 0.1 s (100000 us): 64x2 AMD CPU (CONFIG_HZ=250 / CONFIG_NO_HZ_FULL=y): min diff: 60 us max diff: 831 us mean diff: 135 us 4+2 Mediatek MT8183 CPU (CONFIG_HZ=1000 / CONFIG_NO_HZ_IDLE=y / sysctl kernel.timer_highres=1): min diff: 70 us max diff: 1556 us mean diff: 146 us 4+2 Mediatek MT8183 CPU (CONFIG_HZ=1000 / CONFIG_NO_HZ_IDLE=y / sysctl kernel.timer_highres=0): min diff: 94 us max diff: 7222 us mean diff: 1201 us i.e., maximum 1.5% error, typically ~0.1% error with high resolution timers. Max 7% error, typical 1% error with low resolution timers. This seems reasonable. [1] Stripped / pseudocode: clock_gettime(CLOCK_MONOTONIC, before); nanosleep({ .tv_nsec = usecs * 1000 }, NULL); clock_gettime(CLOCK_MONOTONIC, after); diff = abs((after - before) / 1000 - usecs)); Change-Id: Ifd4821c66c5564f7c975c08769a6742f645e9be0 Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M udelay.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/80808/1 diff --git a/udelay.c b/udelay.c index 0005d39..c42de25 100644 --- a/udelay.c +++ b/udelay.c @@ -220,8 +220,8 @@ /* Precise delay. */ void default_delay(unsigned int usecs) { - /* If the delay is >1 s, use internal_sleep because timing does not need to be so precise. */ - if (usecs > 1000000) { + /* If the delay is >0.1 s, use internal_sleep because timing does not need to be so precise. */ + if (usecs > 100000) { internal_sleep(usecs); } else if (use_clock_gettime) { clock_usec_delay(usecs); -- To view, visit https://review.coreboot.org/c/flashrom/+/80808?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ifd4821c66c5564f7c975c08769a6742f645e9be0 Gerrit-Change-Number: 80808 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-MessageType: newchange

1 0

[XS] Change in flashrom[main]: [RFC] flashrom: Don't throw around "delay 1 second" so lightly
by Brian Norris (Code Review) 01 Mar '24

01 Mar '24

Brian Norris has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/80807?usp=email ) Change subject: [RFC] flashrom: Don't throw around "delay 1 second" so lightly ...................................................................... [RFC] flashrom: Don't throw around "delay 1 second" so lightly Waiting a full second is a very long time, especially when default_delay() chooses to busy-loop. This code has been around for a decade, with vague references to user reports: https://review.coreboot.org/plugins/gitiles/flashrom/+/8ab49e72af8465d4527d… Still, this logic does not belong in the the high-level library logic, used by all programmers and all chips. If there is a timing issue, it should either be encoded in the appropriate programmer or flashchip timing. Change-Id: Ie09651fede3f9f03425244c94a2da8bae00315fc Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M flashrom.c 1 file changed, 0 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/07/80807/1 diff --git a/flashrom.c b/flashrom.c index 630c69d..f4f9e7f 100644 --- a/flashrom.c +++ b/flashrom.c @@ -2330,9 +2330,6 @@ if (verify && !all_skipped) { msg_cinfo("Verifying flash... "); - /* Work around chips which need some time to calm down. */ - programmer_delay(flashctx, 1000*1000); - if (verify_all) combine_image_by_layout(flashctx, newcontents, oldcontents); ret = verify_by_layout(flashctx, verify_layout, curcontents, newcontents); -- To view, visit https://review.coreboot.org/c/flashrom/+/80807?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ie09651fede3f9f03425244c94a2da8bae00315fc Gerrit-Change-Number: 80807 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[main]: udelay: clock_getres() does not provide clock_gettime() resolution
by Brian Norris (Code Review) 01 Mar '24

01 Mar '24

Brian Norris has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/80806?usp=email ) Change subject: udelay: clock_getres() does not provide clock_gettime() resolution ...................................................................... udelay: clock_getres() does not provide clock_gettime() resolution POSIX doesn't give a clear definition of how clock_getres() and clock_gettime() relate, but Linux makes this clearer. See: https://git.kernel.org/linus/01679b5db7172b898be325ff272e10aebd412911 posix-timers: Document sys_clock_getres() correctly The deleted comments are quite clear: "Clock resolution is used to round up timer and interval times, NOT to report clock times" The new comments instead tell how the resolution is only related to clock_settime() and to the precision of *timers* -- not to the precision of clock_gettime() itself. This is particularly relevant depending on the Linux sysctl `sysctl kernel.timer_highres`. When enabled, resolution is always 1 nanosecond; when disabled, it follow the CONFIG_HZ of the kernel in question. But in neither case does this result actually assist determining the quality of the use case in question: busy-looping over clock_gettime(). In practice, any system with a typical CPU clock will have good enough clock_gettime() resolution, and trusting clock_getres() for this is actively misleading (and, wasteful on systems with kernel.timer_highres==0). Any system that doesn't trust the precision of clock_gettime() should simply be building with HAS_CLOCK_GETTIME=no. Therefore, avoid ever looking at clock_getres(), and instead always use clock_gettime() if we're allowed to. This saves a bunch of wasted time in "calibration", which is itself not actually accurate in the presence of hybrid CPUs or CPU DFS. Notably, calibration time-wastage has been the subject of a few past stalled efforts: https://review.coreboot.org/c/flashrom/+/39864 https://review.coreboot.org/c/flashrom/+/44517 BUG=b:325539928 TEST=flashrom never resorts to "Calibrating delay loop" when clock_gettime is available TEST=flashrom works at the same speed (wall clock time) with `sysctl kernel.timer_highres=1` and `sysctl kernel.timer_highres=0` Change-Id: Icdc6e184eacb14d3bd27029e3fc75be4431d82b4 Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M udelay.c 1 file changed, 5 insertions(+), 22 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/80806/1 diff --git a/udelay.c b/udelay.c index 82ddcbd..0005d39 100644 --- a/udelay.c +++ b/udelay.c @@ -27,10 +27,10 @@ #include "flash.h" #include "programmer.h" -static bool use_clock_gettime = false; - #if HAVE_CLOCK_GETTIME == 1 +static const bool use_clock_gettime = true; + #ifdef _POSIX_MONOTONIC_CLOCK static clockid_t clock_id = CLOCK_MONOTONIC; #else @@ -51,28 +51,11 @@ clock_gettime(clock_id, &now); } while (now.tv_sec < end.tv_sec || (now.tv_sec == end.tv_sec && now.tv_nsec < end.tv_nsec)); } - -static int clock_check_res(void) -{ - struct timespec res; - if (!clock_getres(clock_id, &res)) { - if (res.tv_sec == 0 && res.tv_nsec <= 100) { - msg_pinfo("Using clock_gettime for delay loops (clk_id: %d, resolution: %ldns).\n", - (int)clock_id, res.tv_nsec); - use_clock_gettime = true; - return 1; - } - } else if (clock_id != CLOCK_REALTIME && errno == EINVAL) { - /* Try again with CLOCK_REALTIME. */ - clock_id = CLOCK_REALTIME; - return clock_check_res(); - } - return 0; -} #else +static const bool use_clock_gettime = false; + static inline void clock_usec_delay(int usecs) {} -static inline int clock_check_res(void) { return 0; } #endif /* HAVE_CLOCK_GETTIME == 1 */ @@ -135,7 +118,7 @@ void myusec_calibrate_delay(void) { - if (clock_check_res()) + if (use_clock_gettime) return; unsigned long count = 1000; -- To view, visit https://review.coreboot.org/c/flashrom/+/80806?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Icdc6e184eacb14d3bd27029e3fc75be4431d82b4 Gerrit-Change-Number: 80806 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[main]: doc: Add doc how to support flashrom project
by Peter Marheine (Code Review) 28 Feb '24

28 Feb '24

Attention is currently required from: Anastasia Klimchuk, Angel Pons, Stefan Reinauer, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80729?usp=email ) Change subject: doc: Add doc how to support flashrom project ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/80729?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I59a4f5978bc8ffa8ca3a3dc3f15c770ef5fcedce Gerrit-Change-Number: 80729 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Feb 2024 23:08:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: cli_classic: Defer flashrom_init calibration until after options parsing
by Brian Norris (Code Review) 27 Feb '24

27 Feb '24

Brian Norris has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/80772?usp=email ) Change subject: cli_classic: Defer flashrom_init calibration until after options parsing ...................................................................... cli_classic: Defer flashrom_init calibration until after options parsing flashrom_init() may run through an expensive delay calibration phase, depending on the system timer behavior (e.g., if high resolution timers are disabled). This is wholly unnecessary if we're only going to dump usage output or similar. Defer the calibration until we've finished our CLI overhead. BUG=b:325539928 TEST=`flashrom --help` with `sysctl kernel.timer_highres` == 0 Change-Id: I57949ab511df04c6922008fa8cb12467154e2c5e Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M cli_classic.c 1 file changed, 4 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/72/80772/1 diff --git a/cli_classic.c b/cli_classic.c index 60f3fd5..ffded5f 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -917,10 +917,6 @@ print_version(); print_banner(); - /* FIXME: Delay calibration should happen in programmer code. */ - if (flashrom_init(1)) - exit(1); - setbuf(stdout, NULL); parse_options(argc, argv, optstring, long_options, &options); @@ -1005,6 +1001,10 @@ } } + /* FIXME: Delay calibration should happen in programmer code. */ + if (flashrom_init(1)) + exit(1); + if (programmer_init(options.prog, options.pparam)) { msg_perr("Error: Programmer initialization failed.\n"); ret = 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/80772?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I57949ab511df04c6922008fa8cb12467154e2c5e Gerrit-Change-Number: 80772 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-MessageType: newchange

1 0

[XS] Change in flashrom[main]: linux_mtd: Provide no-op delay implementation
by Brian Norris (Code Review) 27 Feb '24

27 Feb '24

Brian Norris has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/80771?usp=email ) Change subject: linux_mtd: Provide no-op delay implementation ...................................................................... linux_mtd: Provide no-op delay implementation Flashrom has several magic programmer_delay() calls scattered throughout its codebase (see cli_classic.s/main() and flashrom.c/flashrom_image_write(), at least). These delays are superfluous for the linux_mtd programmer, because it's an opaque programmer in which all protocol details are handled by the kernel driver. Stub out the delay function, so we don't waste up to 1.1 seconds of needless delay time, depending on the operation and CLI vs libflashrom usage. BUG=b:325539928 Change-Id: I3ac69d3fd7cfc689c5b32c130d044516ed846c29 Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M linux_mtd.c 1 file changed, 9 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/71/80771/1 diff --git a/linux_mtd.c b/linux_mtd.c index 495db9a..eea0cf2 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -429,6 +429,14 @@ return FLASHROM_WP_ERR_RANGE_LIST_UNAVAILABLE; } +static void linux_mtd_nop_delay(const struct flashctx *flash, unsigned int usecs) +{ + /* + * Ignore delay requests. The Linux MTD framework brokers all flash + * protocol, including timing, resets, etc. + */ +} + static const struct opaque_master linux_mtd_opaque_master = { /* max_data_{read,write} don't have any effect for this programmer */ .max_data_read = MAX_DATA_UNSPECIFIED, @@ -441,6 +449,7 @@ .wp_read_cfg = linux_mtd_wp_read_cfg, .wp_write_cfg = linux_mtd_wp_write_cfg, .wp_get_ranges = linux_mtd_wp_get_available_ranges, + .delay = linux_mtd_nop_delay, }; /* Returns 0 if setup is successful, non-zero to indicate error */ -- To view, visit https://review.coreboot.org/c/flashrom/+/80771?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I3ac69d3fd7cfc689c5b32c130d044516ed846c29 Gerrit-Change-Number: 80771 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-MessageType: newchange

1 0

[XS] Change in flashrom[main]: fmap: Update major/minor version check
by Brian Norris (Code Review) 27 Feb '24

27 Feb '24

Attention is currently required from: Anastasia Klimchuk. Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79060?usp=email ) Change subject: fmap: Update major/minor version check ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/79060/comment/9a18351f_755b13f2 : PS2, Line 9: It's not valid to separately check the major and minor versions. The : proper minor check would be something like: : : if (fmap->ver_major == FMAP_VER_MAJOR && : fmap->ver_minor > FMAP_VER_MINOR) Sorry for some delay. I've mostly moved on from this, because the primary bug was that libfmap was using the wrong version. Now that I've fixed that up, everyone I'm aware of is using major=1 minor=1, so none of these ugly comparisons have much more impact at the moment. I guess the real question would be what should happen if anybody ever bumps the minor version for some reason. (Would they still be backward compatible? If so, then why bump it at all?) I'm not aware of any versioning policy for this, but if look at "semantic versioning" (https://semver.org/), I believe their definitions ("Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backward compatible functionality is introduced") would suggest we should ignore minor versions for the purpose of compatibility. > Brian, I like this idea! Maybe you can move the code from commit message into the code? :) The rest of my commit message describes why I don't: because cbfstool only has a major check, and I preferred to not develop a 3rd form of this check. > I looked into the chromiumos patch you linked, it still checks both major and minor versions. Yeah, I got enough run-around here that I fixed my problems elsewhere, and punted on the question of version comparisons. I still think it's bad to leave flashrom as it is, while libfmap was less important. (Or, I can go change the ChromiumOS libfmap much more easily after y'all upstream folks agree on how flashrom and/or cbfstool should look.) > The diff I see is that the check is more strict (both major and minor version expected to be exact equal) while initial fmap.c checks that the versions are not greater than. > > I can agree to make the check more strict, but I don't see why checking minor version has to be dropped, I think it can stay? > > I admit I haven't looked into cbfstool code, is there a way you can link a relevant piece? (if you think it's useful. if not then not) Sure, here you go! https://review.coreboot.org/plugins/gitiles/coreboot/+/354a54ac84a934c1ee60… ``` /* strings containing the magic tend to fail here */ if (fmap->ver_major != FMAP_VER_MAJOR) return 0; ``` Summary reasons for dropping the minor: 1. I was matching cbfstool 2. I don't want a 3rd version of this comparison (perhaps I can change cbfstool too if you'd like) 3. it's unclear what a version bump of any kind would mean w.r.t. compatibility; but minor version changes are considered backwards compatible by some definitions But I don't have strong opinions. At this point, I might just as well drop the CL, as it no longer has much significance despite the existing code being logically strange. -- To view, visit https://review.coreboot.org/c/flashrom/+/79060?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I984835579d3b257a2462906f1f5091b179891bd0 Gerrit-Change-Number: 79060 Gerrit-PatchSet: 2 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Feb 2024 21:35:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: doc: Add doc how to support flashrom project
by Patrick Georgi (Code Review) 27 Feb '24

27 Feb '24

Attention is currently required from: Anastasia Klimchuk, Angel Pons, Peter Marheine, Stefan Reinauer, Thomas Heijligen. Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80729?usp=email ) Change subject: doc: Add doc how to support flashrom project ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/80729?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I59a4f5978bc8ffa8ca3a3dc3f15c770ef5fcedce Gerrit-Change-Number: 80729 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Feb 2024 08:50:02 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit