June 2024 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: tests/: Add programmer enumerate correspondence test
by Edward O'Callaghan (Code Review) 21 Sep '24

21 Sep '24

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47356 ) Change subject: tests/: Add programmer enumerate correspondence test ...................................................................... tests/: Add programmer enumerate correspondence test The enumerate defines within programmer.h must be aligned with the programmer dispatch table within flashrom.c. The following unit-tests tries to improve coverage here to avoid issues like we saw when ENE and MEC1308 were upstreamed. BUG=none BRANCH=none TEST=meson test Change-Id: I13efc4561f743edf51326d4b408be8715b630377 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M tests/flashrom.c M tests/tests.c M tests/tests.h 3 files changed, 94 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/47356/1 diff --git a/tests/flashrom.c b/tests/flashrom.c index 50464dd..9180922 100644 --- a/tests/flashrom.c +++ b/tests/flashrom.c @@ -15,6 +15,47 @@ #include <include/test.h> +/* must be before programmer.h include */ +#define CONFIG_INTERNAL 1 +#define CONFIG_DUMMY 1 +#define CONFIG_MEC1308 1 +#define CONFIG_NIC3COM 1 +#define CONFIG_NICREALTEK 1 +#define CONFIG_NICNATSEMI 1 +#define CONFIG_GFXNVIDIA 1 +#define CONFIG_RAIDEN 1 +#define CONFIG_DRKAISER 1 +#define CONFIG_SATASII 1 +#define CONFIG_ATAHPT 1 +#define CONFIG_ATAVIA 1 +#define CONFIG_ATAPROMISE 1 +#define CONFIG_IT8212 1 +#define CONFIG_FT2232_SPI 1 +#define CONFIG_SERPROG 1 +#define CONFIG_BUSPIRATE_SPI 1 +#define CONFIG_DEDIPROG 1 +#define CONFIG_DEVELOPERBOX_SPI 1 +#define CONFIG_ENE_LPC 1 +#define CONFIG_RAYER_SPI 1 +#define CONFIG_PONY_SPI 1 +#define CONFIG_NICINTEL 1 +#define CONFIG_NICINTEL_SPI 1 +#define CONFIG_NICINTEL_EEPROM 1 +#define CONFIG_OGP_SPI 1 +#define CONFIG_SATAMV 1 +#define CONFIG_LINUX_MTD 1 +#define CONFIG_LINUX_SPI 1 +#define CONFIG_LSPCON_I2C_SPI 1 +#define CONFIG_REALTEK_MST_I2C_SPI 1 +#define CONFIG_USBBLASTER_SPI 1 +#define CONFIG_MSTARDDC_SPI 1 +#define CONFIG_PICKIT2_SPI 1 +#define CONFIG_CH341A_SPI 1 +#define CONFIG_DIGILENT_SPI 1 +#define CONFIG_JLINK_SPI 1 +#define CONFIG_NI845X_SPI 1 +#define CONFIG_STLINKV3_SPI 1 + #include "programmer.h" void flashbuses_to_text_test_success(void **state) @@ -49,3 +90,54 @@ bustype = BUS_NONE; assert_string_equal(flashbuses_to_text(bustype), "None"); } + +void validate_prog_enumerates_test_success(void **state) +{ + (void) state; /* unused */ + + enum programmer prog = PROGRAMMER_INTERNAL; + const char *prog_names[] = { + "internal", + "dummy", + "mec1308", + "nic3com", + "nicrealtek", + "nicnatsemi", + "gfxnvidia", + "raiden_debug", + "drkaiser", + "satasii", + "atahpt", + "atavia", + "atapromise", + "it8212", + "ft2232_spi", + "serprog", + "buspirate_spi", + "dediprog", + "developerbox", + "ene_lpc", + "rayer_spi", + "pony_spi", + "nicintel", + "nicintel_spi", + "nicintel_eeprom", + "ogp_spi", + "satamv", + "linux_mtd", + "linux_spi", + "lspcon_i2c_spi", + "realtek_mst_i2c_spi", + "usbblaster_spi", + "mstarddc_spi", + "pickit2_spi", + "ch341a_spi", + "digilent_spi", + "jlink_spi", + "ni845x_spi", + "stlinkv3_spi", + }; + for (; prog < PROGRAMMER_INVALID; prog++) { + assert_string_equal(programmer_table[prog].name, prog_names[prog]); + } +} diff --git a/tests/tests.c b/tests/tests.c index 3124607..d2a22a5 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -46,6 +46,7 @@ const struct CMUnitTest flashrom_tests[] = { cmocka_unit_test(flashbuses_to_text_test_success), + cmocka_unit_test(validate_prog_enumerates_test_success), }; ret |= cmocka_run_group_tests_name("flashrom.c tests", flashrom_tests, NULL, NULL); diff --git a/tests/tests.h b/tests/tests.h index cb905fd..89fc7fa 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -27,6 +27,7 @@ /* flashrom.c */ void flashbuses_to_text_test_success(void **state); +void validate_prog_enumerates_test_success(void **state); /* spi25.c */ void spi_write_enable_test_success(void **state); -- To view, visit https://review.coreboot.org/c/flashrom/+/47356 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I13efc4561f743edf51326d4b408be8715b630377 Gerrit-Change-Number: 47356 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

6 8

Change in flashrom[master]: Makefile: Check for last line only from preprocessed output
by Khem Raj (Code Review) 01 Aug '24

01 Aug '24

Khem Raj has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43770 ) Change subject: Makefile: Check for last line only from preprocessed output ...................................................................... Makefile: Check for last line only from preprocessed output This started to fail with glibc 2.32 since glibc added additional attributes to functions in signal.h therefore existing regexp started to fail as it is not able to handle these functions e.g. extern int siginterrupt (int __sig, int __interrupt) __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__deprecated__ ("Use sigaction with SA_RESTART instead"))); grep -v '^\#' | grep '"' | cut -f 2 -d'"' bit outside of fd_set selected Use sigaction with SA_RESTART instead arm So changing it to tail -1 | grep '"' | cut -f 2 -d'"' arm Produces the expected result, this was hidden until now Signed-off-by: Khem Raj <raj.khem(a)gmail.com> Change-Id: I123a046e142d54632f12d54e2aa09b0928c02b91 --- M Makefile 1 file changed, 3 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/70/43770/1 diff --git a/Makefile b/Makefile index 803529f..3795681 100644 --- a/Makefile +++ b/Makefile @@ -106,7 +106,7 @@ # IMPORTANT: The following line must be placed before TARGET_OS is ever used # (of course), but should come after any lines setting CC because the line # below uses CC itself. -override TARGET_OS := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E os.h 2>/dev/null | grep -v '^\#' | grep '"' | cut -f 2 -d'"')) +override TARGET_OS := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E os.h 2>/dev/null | tail -1 | grep '"' | cut -f 2 -d'"')) ifeq ($(TARGET_OS), Darwin) override CPPFLAGS += -I/opt/local/include -I/usr/local/include @@ -460,8 +460,8 @@ # IMPORTANT: The following line must be placed before ARCH is ever used # (of course), but should come after any lines setting CC because the line # below uses CC itself. -override ARCH := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E archtest.c 2>/dev/null | grep -v '^\#' | grep '"' | cut -f 2 -d'"')) -override ENDIAN := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E endiantest.c 2>/dev/null | grep -v '^\#')) +override ARCH := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E archtest.c 2>/dev/null | tail -1 | grep '"' | cut -f 2 -d'"')) +override ENDIAN := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E endiantest.c 2>/dev/null | tail -1)) # Disable the internal programmer on unsupported architectures (everything but x86 and mipsel) ifneq ($(ARCH)-little, $(filter $(ARCH),x86 mips)-$(ENDIAN)) -- To view, visit https://review.coreboot.org/c/flashrom/+/43770 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I123a046e142d54632f12d54e2aa09b0928c02b91 Gerrit-Change-Number: 43770 Gerrit-PatchSet: 1 Gerrit-Owner: Khem Raj Gerrit-MessageType: newchange

4 5

Change in flashrom[master]: Add -- to make clean
by Ryan O'Leary (Code Review) 01 Aug '24

01 Aug '24

Ryan O'Leary has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/37776 ) Change subject: Add -- to make clean ...................................................................... Add -- to make clean Somehow, I ended up with a file name "-c.d". I probably made it by mistake. This change lets "make clean" be a bit more resilient to files such as these. Change-Id: I2517ffac975f3df75f706350a07f189a98a11b7c Signed-off-by: Ryan O'Leary <ryanoleary(a)google.com> --- M Makefile 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/37776/1 diff --git a/Makefile b/Makefile index 518d41b..fa66910 100644 --- a/Makefile +++ b/Makefile @@ -1129,7 +1129,7 @@ # This includes all frontends and libflashrom. # We don't use EXEC_SUFFIX here because we want to clean everything. clean: - rm -f $(PROGRAM) $(PROGRAM).exe libflashrom.a *.o *.d $(PROGRAM).8 $(PROGRAM).8.html $(BUILD_DETAILS_FILE) + rm -f -- $(PROGRAM) $(PROGRAM).exe libflashrom.a *.o *.d $(PROGRAM).8 $(PROGRAM).8.html $(BUILD_DETAILS_FILE) @+$(MAKE) -C util/ich_descriptors_tool/ clean distclean: clean -- To view, visit https://review.coreboot.org/c/flashrom/+/37776 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2517ffac975f3df75f706350a07f189a98a11b7c Gerrit-Change-Number: 37776 Gerrit-PatchSet: 1 Gerrit-Owner: Ryan O'Leary <ryanoleary(a)google.com> Gerrit-MessageType: newchange

6 8

[S] Change in flashrom[main]: ch347_spi: Add spi clock frequency selection
by Nicholas Chin (Code Review) 29 Jun '24

29 Jun '24

Attention is currently required from: Anastasia Klimchuk, ZhiYuanNJ. Nicholas Chin has posted comments on this change by ZhiYuanNJ. ( https://review.coreboot.org/c/flashrom/+/82776?usp=email ) Change subject: ch347_spi: Add spi clock frequency selection ...................................................................... Patch Set 7: (1 comment) File ch347_spi.c: https://review.coreboot.org/c/flashrom/+/82776/comment/5aea48e4_45ce6a7b?us… : PS7, Line 365: 30MHz > It seems you are changing the default, why? […] I don't mind changing the default if it generally works at 30 MHz. I had set it to 15 MHz before as a fairly conservative value as you couldn't easily change it, but that's less of a concern with the ability to change it. That said, I just did some testing and found that a few of my flash chips (Winbond W25Q128JV, Eon EN25QH128A) don't seem to work reliably with the CH347T at 30 MHz, so it might be good to keep it at 15 MHz. The only one that seemed to work reliably was the Macronix MX25L1606E. Sometimes the Winbond chip would work, but when trying to verify it, it would fail, and the differences seemed to be single bit flips. All of those should work at that frequency according to the datasheet, so it's probably some setup/hold time violation. These chips do work with an FT232H programmer I have with a divisor of 2 (30 MHz). -- To view, visit https://review.coreboot.org/c/flashrom/+/82776?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If2be48929db540a6598ac0b60b37e64597156db7 Gerrit-Change-Number: 82776 Gerrit-PatchSet: 7 Gerrit-Owner: ZhiYuanNJ Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: ZhiYuanNJ Gerrit-Comment-Date: Sat, 29 Jun 2024 13:10:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: tests/selfcheck.c: Include the missing header lifecycle.h
by Anastasia Klimchuk (Code Review) 29 Jun '24

29 Jun '24

Attention is currently required from: Hsuan Ting Chen, Hsuan-ting Chen. Anastasia Klimchuk has posted comments on this change by Hsuan-ting Chen. ( https://review.coreboot.org/c/flashrom/+/83219?usp=email ) Change subject: tests/selfcheck.c: Include the missing header lifecycle.h ...................................................................... Patch Set 1: Code-Review-1 (3 comments) Patchset: PS1: I believe you that the issue is real, but I think the solution is not correct. We need to find the right solution. PS1: > as long as we need it, we should include it We don't need lifecycle.h here :) Let's debug the issue properly. I am super suspicious of this: > error: type of ‘selfcheck_board_matches_table’ defaults to ‘int’ [-Werror=implicit-int] First of all , which options did you use for compiling? CHROMIUM repository might have slightly different compiler options set up. Also, was CONFIG_INTERNAL enabled? Commit Message: https://review.coreboot.org/c/flashrom/+/83219/comment/35a145f0_6dc67b75?us… : PS1, Line 9: selfcheck.c is using the macro SKIP_TEST, so we should include the : header lifecycle.h SKIP_TEST is defined in `include/test.h` , which is already included. `lifecycle.h` is not relevant to SKIP_TEST neither to selfcheck. If you look into lifecycle.h you can see it has functions for programmer lifecycle, this is why all programmer tests have it. Here it is not needed. I believe you that you see an error on your environment, but lifecycle.h is not the solution. We need to figure out why exactly the error is happening. -- To view, visit https://review.coreboot.org/c/flashrom/+/83219?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ifc07ac66320712fdbf31504b9a980354c1883f40 Gerrit-Change-Number: 83219 Gerrit-PatchSet: 1 Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Comment-Date: Sat, 29 Jun 2024 09:23:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Hsuan-ting Chen <roccochen(a)google.com>

1 0

[S] Change in flashrom[main]: ch347_spi: Add spi clock frequency selection
by Anastasia Klimchuk (Code Review) 29 Jun '24

29 Jun '24

Attention is currently required from: ZhiYuanNJ. Anastasia Klimchuk has posted comments on this change by ZhiYuanNJ. ( https://review.coreboot.org/c/flashrom/+/82776?usp=email ) Change subject: ch347_spi: Add spi clock frequency selection ...................................................................... Patch Set 7: (5 comments) Patchset: PS7: Thanks for adding new feature! I gave some comments. File ch347_spi.c: https://review.coreboot.org/c/flashrom/+/82776/comment/52b187d8_ca5593e1?us… : PS7, Line 355: arg = extract_programmer_param_str(cfg, "spispeed"); Since you are adding a param, this needs to be explained on manpage for the programmer. Manpage source code is located here: doc/classic_cli_manpage.rst (on the website it looks like this https://flashrom.org/classic_cli_manpage.html#ch347-spi-programmer) You need to update the section for ch347_spi You can test your changes locally, the information is here: https://flashrom.org/how_to_add_docs.html (or you can view the same doc in the tree in your repo) https://review.coreboot.org/c/flashrom/+/82776/comment/448ec752_e54e847e?us… : PS7, Line 357: index = 0 Let's use different variable name, index is already used for indexing the device table. `speed_index` or something like that, but not the same variable name. As a concrete example how things will go wrong: if spispeed param is not provided, you won't even reach this line of code (because arg is NULL). So your index will be whatever is initialized from device table - which is completely different table. You need its own variable for spispeed index, give it default value, and use the same default value on line 366 for the case when spispeed param is unknown value. https://review.coreboot.org/c/flashrom/+/82776/comment/7421829d_502fefb5?us… : PS7, Line 365: Invalid SPI speed It would be invalid value if you would throw an error and exit, however you do not, but instead continue with default value. So I think the better name is "unknown" instead of "invalid". I suggest > Unknown value of spispeed parameter, using default XXX clock spi https://review.coreboot.org/c/flashrom/+/82776/comment/5537ffa6_defad3df?us… : PS7, Line 365: 30MHz It seems you are changing the default, why? Previously the divisor was fixed to 2, which corresponds to 15M with index=2 Now it will be 1, when spispeed value is not recognized? -- To view, visit https://review.coreboot.org/c/flashrom/+/82776?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If2be48929db540a6598ac0b60b37e64597156db7 Gerrit-Change-Number: 82776 Gerrit-PatchSet: 7 Gerrit-Owner: ZhiYuanNJ Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: ZhiYuanNJ Gerrit-Comment-Date: Sat, 29 Jun 2024 08:56:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in flashrom[main]: ch347_spi: Add spi clock frequency selection
by Nicholas Chin (Code Review) 29 Jun '24

29 Jun '24

Attention is currently required from: ZhiYuanNJ. Nicholas Chin has posted comments on this change by ZhiYuanNJ. ( https://review.coreboot.org/c/flashrom/+/82776?usp=email ) Change subject: ch347_spi: Add spi clock frequency selection ...................................................................... Patch Set 7: (1 comment) File ch347_spi.c: https://review.coreboot.org/c/flashrom/+/82776/comment/df88a6ff_14ffc34a?us… : PS7, Line 372: clock spi "clock spi" is redundant here as clock is already mentioned earlier. ```suggestion msg_pinfo("CH347 SPI clock set to %sHz.\n", spispeeds[index].name); ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/82776?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If2be48929db540a6598ac0b60b37e64597156db7 Gerrit-Change-Number: 82776 Gerrit-PatchSet: 7 Gerrit-Owner: ZhiYuanNJ Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: ZhiYuanNJ Gerrit-Comment-Date: Sat, 29 Jun 2024 01:55:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[XS] Change in flashrom[main]: dummyflasher: assign address of function for LLVM
by Fabian Groffen (Code Review) 28 Jun '24

28 Jun '24

Fabian Groffen has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/83262?usp=email ) Change subject: dummyflasher: assign address of function for LLVM ...................................................................... dummyflasher: assign address of function for LLVM ../flashrom-v1.3.0/dummyflasher.c:933:18: error: incompatible function pointer types initializing 'bool (*)(const struct flashrom_flashctx *, uint8_t)' (aka 'bool (*)(const struct flashrom_flashctx *, unsigned char)') with an expression of type 'bool (struct flashrom_flashctx *, uint8_t)' (aka 'bool (struct flashrom_flashctx *, unsigned char)') [-Wincompatible-function-pointer-types] https://bugs.gentoo.org/915617 Change-Id: I8e5c9ea014d40a2e9e0621b25f8c404e80a79952 Signed-off-by: Fabian Groffen <grobian(a)gentoo.org> --- M dummyflasher.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/83262/1 diff --git a/dummyflasher.c b/dummyflasher.c index cf4ca03..cbf1e49 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -969,7 +969,7 @@ .read = default_spi_read, .write_256 = dummy_spi_write_256, .shutdown = dummy_shutdown, - .probe_opcode = dummy_spi_probe_opcode, + .probe_opcode = &dummy_spi_probe_opcode, .delay = dummy_nop_delay, }; -- To view, visit https://review.coreboot.org/c/flashrom/+/83262?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8e5c9ea014d40a2e9e0621b25f8c404e80a79952 Gerrit-Change-Number: 83262 Gerrit-PatchSet: 1 Gerrit-Owner: Fabian Groffen <grobian(a)gentoo.org>

1 0

[XS] Change in flashrom[main]: libflashrom.map: remove non-existant functions
by Fabian Groffen (Code Review) 28 Jun '24

28 Jun '24

Fabian Groffen has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/83261?usp=email ) Change subject: libflashrom.map: remove non-existant functions ...................................................................... libflashrom.map: remove non-existant functions Remove symbol names from the map that do not exist in the code. https://bugs.gentoo.org/928955 Change-Id: I30bab842d9cbd2daaa9902fd3223f47145cb0e7f Signed-off-by: Fabian Groffen <grobian(a)gentoo.org> --- M libflashrom.map 1 file changed, 0 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/61/83261/1 diff --git a/libflashrom.map b/libflashrom.map index 77abf72..e1cdfa6 100644 --- a/libflashrom.map +++ b/libflashrom.map @@ -1,7 +1,5 @@ LIBFLASHROM_1.0 { global: - flashrom_board_info; - flashrom_chipset_info; flashrom_data_free; flashrom_flag_get; flashrom_flag_set; @@ -9,7 +7,6 @@ flashrom_flash_getsize; flashrom_flash_probe; flashrom_flash_release; - flashrom_flashchip_info; flashrom_image_read; flashrom_image_verify; flashrom_image_write; -- To view, visit https://review.coreboot.org/c/flashrom/+/83261?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I30bab842d9cbd2daaa9902fd3223f47145cb0e7f Gerrit-Change-Number: 83261 Gerrit-PatchSet: 1 Gerrit-Owner: Fabian Groffen <grobian(a)gentoo.org>

1 0

[S] Change in flashrom[main]: how_to_add_new_chip: Add a section for feature bits and WRSR handling
by Hsuan-ting Chen (Code Review) 28 Jun '24

28 Jun '24

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen. Hsuan-ting Chen has posted comments on this change by Hsuan-ting Chen. ( https://review.coreboot.org/c/flashrom/+/82908?usp=email ) Change subject: how_to_add_new_chip: Add a section for feature bits and WRSR handling ...................................................................... Patch Set 2: (10 comments) Patchset: PS2: Thanks for the suggestions! File doc/contrib_howtos/how_to_add_new_chip.rst: https://review.coreboot.org/c/flashrom/+/82908/comment/0085c63d_1e94e19b?us… : PS1, Line 105: Feature Bits : ------------- > This can be on the same level as "Properties", especially because you have future plans to gradually […] Done https://review.coreboot.org/c/flashrom/+/82908/comment/57b8609c_1770d64c?us… : PS1, Line 108: The ``.feature_bits`` field in a chip definition encodes various features using a bitmask. : Here are some of the selected feature bits that need to be highlighted: > With this, we don't need an item in a list inside Properties. […] Done https://review.coreboot.org/c/flashrom/+/82908/comment/1a3333b7_a079cd5e?us… : PS1, Line 111: Write-Status-Register (WRSR) Handling : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > This header also goes on level up, use `-` Done https://review.coreboot.org/c/flashrom/+/82908/comment/c2b4d574_a09e6bf8?us… : PS1, Line 113: : ``FEATURE_WRSR_EWSR``, ``FEATURE_WRSR_WREN``, and ``FEATURE_WRSR_EITHER``. These bits used for **SPI only**. > I think this sentence is not needed. […] Done AFAIK it should be SPI only. Move the "SPI only" hints into the next section instead. https://review.coreboot.org/c/flashrom/+/82908/comment/98d08826_969aedc1?us… : PS1, Line 120: This > Delete "This", start with "Indicates that" Done I noticed that it actually doesn't jump to a newline after EWSR, e.g. ``` FEATURE_WRSR_EWSR indicates that we need an Enable-Write-Status-Register (EWSR) instruction which opens the status register for the immediately-followed next WRSR instruction. Usually, the opcode is 0x50. ``` So I used the "indicates" with lowercase and remove the ":" https://review.coreboot.org/c/flashrom/+/82908/comment/c85d39c7_8982b7d3?us… : PS1, Line 124: This > same Done https://review.coreboot.org/c/flashrom/+/82908/comment/74fae8b8_7f28dc8b?us… : PS1, Line 125: priort > typo, prior Done https://review.coreboot.org/c/flashrom/+/82908/comment/cef552ed_32a04ace?us… : PS1, Line 128: This > same Done https://review.coreboot.org/c/flashrom/+/82908/comment/09a6fb9b_ea72a571?us… : PS1, Line 110: : Write-Status-Register (WRSR) Handling : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ : : ``FEATURE_WRSR_EWSR``, ``FEATURE_WRSR_WREN``, and ``FEATURE_WRSR_EITHER``. These bits used for **SPI only**. : : The Write Status Register (WRSR) is used to configure various settings within the flash chip, including write protection and : other features. The way WRSR is accessed varies between SPI flash chips, leading to the need for these feature bits. : : * ``FEATURE_WRSR_EWSR``: : This indicates that we need an **Enable-Write-Status-Register** (EWSR) instruction which opens the status register for the : immediately-followed next WRSR instruction. Usually, the opcode is **0x50**. : : * ``FEATURE_WRSR_WREN``: : This indicates that we need an **Write-Enable** (WREN) instruction to set the Write Enable Latch (WEL) bit. The WEL bit : must be set priort to every WRSR command. Usually, the opcode is **0x06**. : : * ``FEATURE_WRSR_EITHER``: : This indicates that either EWSR or WREN is supported in this chip. > Thank you for explaining! It's really good that I know your future plans. […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/82908?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I34c20933a375380c8702f79ac637595cd3466000 Gerrit-Change-Number: 82908 Gerrit-PatchSet: 2 Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.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-CC: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 28 Jun 2024 10:12:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Hsuan-ting Chen <roccochen(a)google.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2024