March 2025 - flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[main]: flashchips: Add XMC XM25QH64A
by Anastasia Klimchuk (Code Review) 26 Mar '25

26 Mar '25

Attention is currently required from: Eric Park, Nikolai Artemiev, Stefan Reinauer. Anastasia Klimchuk has posted comments on this change by Eric Park. ( https://review.coreboot.org/c/flashrom/+/86990?usp=email ) Change subject: flashchips: Add XMC XM25QH64A ...................................................................... Patch Set 6: (4 comments) Patchset: PS1: > Sorry this took so long. […] Thanks so much for testing, I appreciate it. WP is write protection, but it's fine to add initial support without WP. Patchset: PS4: > Thank you for the comments! I've modified the commit with the changes. […] Great work! I have two last small comments and that should be it :) Commit Message: https://review.coreboot.org/c/flashrom/+/86990/comment/7963c326_88ddcd01?us… : PS6, Line 14: I forgot to ask: would you please add link to datasheet in commit message? I found one at https://www.micros.com.pl/mediaserver/PFXM25QH64AHIG_0001.pdf but if you have other public link it's fine too. File flashchips/xmc.c: https://review.coreboot.org/c/flashrom/+/86990/comment/0b0c1b85_599838c2?us… : PS6, Line 337: TEST_OK_PREW Based on what you explained in the other comment (about testing), the test status should be `TEST_OK_PR` (which means tested Probe and Reading). E is erase, W is write, B is WP or write-protection, but as I understand you were not performing those operations. It's fine to have chip partially tested: if later someone does erase/write successfully, test status will be updated. -- To view, visit https://review.coreboot.org/c/flashrom/+/86990?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: I5b11e30f0a5357a6cbb32ddb93f450de5364c60b Gerrit-Change-Number: 86990 Gerrit-PatchSet: 6 Gerrit-Owner: Eric Park <me(a)ericswpark.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Eric Park <me(a)ericswpark.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Eric Park <me(a)ericswpark.com> Gerrit-Comment-Date: Wed, 26 Mar 2025 09:15:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Eric Park <me(a)ericswpark.com>

1 0

[M] Change in flashrom[main]: libflashrom: Update the API for Logger Callback
by Dmitry Zhadinets (Code Review) 26 Mar '25

26 Mar '25

Attention is currently required from: Anastasia Klimchuk, Peter Marheine. Dmitry Zhadinets has posted comments on this change by Dmitry Zhadinets. ( https://review.coreboot.org/c/flashrom/+/86875?usp=email ) Change subject: libflashrom: Update the API for Logger Callback ...................................................................... Patch Set 7: (1 comment) File libflashrom.c: https://review.coreboot.org/c/flashrom/+/86875/comment/e93faea2_acdf524f?us… : PS6, Line 84: if (!log_callback && global_log_callback == format_message_and_invoke_log_callback_v2) : global_log_callback = NULL; > Great review helps finding bugs. This is the case. Thx […] *The logic of this condition was to reset log callback v**1** only in the case if it was installed by set_v2 op -- To view, visit https://review.coreboot.org/c/flashrom/+/86875?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: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Gerrit-Change-Number: 86875 Gerrit-PatchSet: 7 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 26 Mar 2025 01:11:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Dmitry Zhadinets <dzhadinets(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: libflashrom: Update the API for Logger Callback
by Dmitry Zhadinets (Code Review) 26 Mar '25

26 Mar '25

Attention is currently required from: Anastasia Klimchuk, Peter Marheine. Dmitry Zhadinets has posted comments on this change by Dmitry Zhadinets. ( https://review.coreboot.org/c/flashrom/+/86875?usp=email ) Change subject: libflashrom: Update the API for Logger Callback ...................................................................... Patch Set 6: (3 comments) File include/libflashrom.h: https://review.coreboot.org/c/flashrom/+/86875/comment/6eb6957b_0618082a?us… : PS4, Line 83: * > I was trying to find it, but I couldn't, but then I found the addition at `flashrom_set_progress_cal […] It was not me. the man in the middle :) sorry. fixed File libflashrom.c: https://review.coreboot.org/c/flashrom/+/86875/comment/1d9c4611_5c62c5da?us… : PS4, Line 59: /* Dynamic messages and double formatting may not be ideal, : as they are slow. It would be better to limit the message : size to a fixed value (e.g., char message[1024] = {0};). */ > I found one very long message in cli_classic. […] Done File libflashrom.c: https://review.coreboot.org/c/flashrom/+/86875/comment/d06b0536_548c8263?us… : PS6, Line 84: if (!log_callback && global_log_callback == format_message_and_invoke_log_callback_v2) : global_log_callback = NULL; > I spent some time looking at this, then I figured this is doing a reset (when log_callback is passed […] Great review helps finding bugs. This is the case. Thx Looks like I had a bad day. The logic of this condition was to reset log callback v2 only in the case if it was installed by set_v2 op We have following logic. Better with examples set_log_cb(cb_v1) /* will set v1 cb and replace v2 if it was set */ set_log_cb_v2(cb_v2) /* set v2 cb and replace v1 */ set_log_cb(NULL) /* reset everything */ set_log_cb_v2(NULL) /* reset v2 cb only. because it should not affect previous API behavior */ The public API must be reliable even if the usage is crazy. This why I wanted to add tests with a mix of both versions of set_log_cb... The corner cases: 1: set_log_cb_v2(cb_v2) /* set using v2 */ set_log_cb(NULL) /* reset using v1 */ it was working 2: set_log_cb(cb_v1) /* set using v1 */ set_log_cb_v2(NULL) /* reset using v2 */ it was an issue. set_v2 did reset of cb_v2 but installed inner cb and replaced v1 My initial implementation was like you suggested. But then I thought my v2 should affect v1 behavior In your implementation set_v1/v2(NULL) will always reset everything In my one we will have following set_log_cb(cb_v1) /* set using v1 */ set_log_cb_v2(NULL) /* will do nothing cb_v1 will continue working */ So I will implement as you will say. Let's see one more round -- To view, visit https://review.coreboot.org/c/flashrom/+/86875?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: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Gerrit-Change-Number: 86875 Gerrit-PatchSet: 6 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 26 Mar 2025 01:03:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Dmitry Zhadinets <dzhadinets(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>

1 0

[M] Change in flashrom[main]: libflashrom: Update the API for Logger Callback
by Dmitry Zhadinets (Code Review) 26 Mar '25

26 Mar '25

Attention is currently required from: Dmitry Zhadinets. Hello Anastasia Klimchuk, Peter Marheine, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/86875?usp=email to look at the new patch set (#7). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: libflashrom: Update the API for Logger Callback ...................................................................... libflashrom: Update the API for Logger Callback The initial implementation does not account for user_data, requiring the calling application to use a global scope. This may lead to issues related to object lifecycle management and other architectural concerns. This patch adds user_data to the user’s log callback. Moreover, it performs message formatting, so the application only needs to pass the formatted string to the selected output. The change does not break the existing logging API but extends it. A new API version is introduced with the v2 suffix. Testing: Both unit tests and CLI tools serve as libflashrom clients. All unit tests run successfully. Change-Id: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Signed-off-by: Dmitry Zhadinets <dzhadinets(a)gmail.com> --- M include/libflashrom.h M libflashrom.c M libflashrom.map A tests/libflashrom.c M tests/meson.build M tests/tests.c M tests/tests.h 7 files changed, 141 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/75/86875/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/86875?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Gerrit-Change-Number: 86875 Gerrit-PatchSet: 7 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dmitry Zhadinets <dzhadinets(a)gmail.com>

1 0

[XS] Change in flashrom[main]: flashchips/gigadevice: Add GD25B127D
by Victor Lim (Code Review) 25 Mar '25

25 Mar '25

Victor Lim has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/86996?usp=email ) Change subject: flashchips/gigadevice: Add GD25B127D ...................................................................... flashchips/gigadevice: Add GD25B127D GD25B127D: 3V 128Mbit https://www.gigadevice.com.cn/Public/Uploads/uploadfile/files/20220714/DS-0… Change-Id: I0851a9b3112cb3aaec816b6ef2b4dad5a404d096 Signed-off-by: Victor Lim <vlim(a)gigadevice.com> --- M flashchips/gigadevice.c M include/flashchips.h 2 files changed, 3 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/86996/1 diff --git a/flashchips/gigadevice.c b/flashchips/gigadevice.c index 12b24b0..c1ad83d 100644 --- a/flashchips/gigadevice.c +++ b/flashchips/gigadevice.c @@ -794,7 +794,7 @@ { .vendor = "GigaDevice", - .name = "GD25Q128E/GD25B128E/GD25R128E/GD25Q127C", + .name = "GD25Q128E/GD25B128E/GD25R128E/GD25Q127C/GD25B127D", .bustype = BUS_SPI, .manufacture_id = GIGADEVICE_ID, .model_id = GIGADEVICE_GD25Q128, @@ -802,7 +802,7 @@ .page_size = 256, /* OTP: 3072B total; read 0x48; write 0x42, erase 0x44 */ .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_WRSR2, - .tested = TEST_OK_PREWB, + .tested = TEST_UNTESTED, .probe = PROBE_SPI_RDID, .probe_timing = TIMING_ZERO, .block_erasers = diff --git a/include/flashchips.h b/include/flashchips.h index 556729c..6e60864 100644 --- a/include/flashchips.h +++ b/include/flashchips.h @@ -394,7 +394,7 @@ #define GIGADEVICE_GD25Q16 0x4015 /* Same as GD25Q16B (which has OTP) */ #define GIGADEVICE_GD25Q32 0x4016 /* Same as GD25Q32B */ #define GIGADEVICE_GD25Q64 0x4017 /* Same as GD25Q64B */ -#define GIGADEVICE_GD25Q128 0x4018 /* Same as GD25Q128B, GD25Q127C, GD25Q128C,and GD25Q128E, GD25B128E, GD25R128E can be distinguished by SFDP */ +#define GIGADEVICE_GD25Q128 0x4018 /* Same as GD25Q128B, GD25Q127C, GD25Q128C, GD25B127D, GD25Q128E, GD25B128E, GD25R128E can be distinguished by SFDP */ #define GIGADEVICE_GD25Q256D 0x4019 /* Same as GD25B256E, GD25Q256E, GD25R256E */ #define GIGADEVICE_GD25B512MF 0x401A /* Same as GD25R512MF */ #define GIGADEVICE_GD25VQ21B 0x4212 -- To view, visit https://review.coreboot.org/c/flashrom/+/86996?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: I0851a9b3112cb3aaec816b6ef2b4dad5a404d096 Gerrit-Change-Number: 86996 Gerrit-PatchSet: 1 Gerrit-Owner: Victor Lim <vlim(a)gigadevice.com>

1 0

[S] Change in flashrom[main]: flashchips: Add XMC XM25QH64A
by Eric Park (Code Review) 25 Mar '25

25 Mar '25

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer. Eric Park has posted comments on this change by Eric Park. ( https://review.coreboot.org/c/flashrom/+/86990?usp=email ) Change subject: flashchips: Add XMC XM25QH64A ...................................................................... Patch Set 6: Code-Review+1 (6 comments) Patchset: PS1: Sorry this took so long. I was initially trying to test on my Windows machine, but for some reason flashrom+MSYS2-MINGW64+my CH341A programmer weren't playing nicely together and would give me some programmer initialization error. I moved over to a NixOS system and rebuilt flashrom and it worked perfectly there on the first try. I have a corpus of a Chinese CCTV camera with that chip so I tried reading off of it twice, and both binary files share the same MD5sum. Running strings show snippets from the Chinese firmware with "Tuya"-related strings on it so the data I'm getting back seems to be sound. Flashrom did warn me that "WP" was untested on this flash chip, which I'm guessing is the writing/erase portion. Flashrom also correctly identified the chip model. Patchset: PS4: > Eric, thank you for the patch! I added a few comments. […] Thank you for the comments! I've modified the commit with the changes. Please see comment above regarding testing. Commit Message: https://review.coreboot.org/c/flashrom/+/86990/comment/0777d42e_2eeb5e5d?us… : PS4, Line 7: XMC25QH64A > Let's be exact so that it's searchable, call it `XMC XM25QH64A` Done https://review.coreboot.org/c/flashrom/+/86990/comment/ec8dfb59_2d1e1a81?us… : PS4, Line 18: Signed-off-by: Eric Park <me(a)ericswpark.com> > Add yourself too as Co-authored-by (keep the sign off line too) Done File flashchips/xmc.c: https://review.coreboot.org/c/flashrom/+/86990/comment/41536c11_896e15f5?us… : PS4, Line 251: .name = "XM25QH64A", > I would put this entry just before XM25QH128A in this file too (same as their ids are ordered), woul […] Done File include/flashchips.h: https://review.coreboot.org/c/flashrom/+/86990/comment/3579fc5d_f8396bcb?us… : PS4, Line 1085: #define XMC_ID 0x20 /* same as ST_ID */ : #define XMC_XM25QH256B 0x6019 : #define XMC_XM25QH128B 0x6018 : #define XMC_XM25QH64A 0x7017 : #define XMC_XM25QH64B 0x6017 : #define XMC_XM25QH32B 0x4016 /* 0x6016 for QPI mode */ : #define XMC_XM25QH16B 0x4015 : #define XMC_XM25QH80B 0x4014 : #define XMC_XM25QH40B 0x4013 : #define XMC_XM25QU256B 0x7019 : #define XMC_XM25QU128B 0x5018 : #define XMC_XM25QU64A 0x3817 : #define XMC_XM25QU64B 0x5017 : #define XMC_XM25QU40B 0x5013 : #define XMC_XM25QU20B 0x5012 > I know it was like this in the original pull request, but we don't need to do the aliasing. […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/86990?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: I5b11e30f0a5357a6cbb32ddb93f450de5364c60b Gerrit-Change-Number: 86990 Gerrit-PatchSet: 6 Gerrit-Owner: Eric Park <me(a)ericswpark.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Eric Park <me(a)ericswpark.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 25 Mar 2025 17:06:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[S] Change in flashrom[main]: flashchips: Add XMC XM25QH64A
by Eric Park (Code Review) 25 Mar '25

25 Mar '25

Attention is currently required from: Eric Park, Nikolai Artemiev, Stefan Reinauer. Hello Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/86990?usp=email to look at the new patch set (#6). The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) Change subject: flashchips: Add XMC XM25QH64A ...................................................................... flashchips: Add XMC XM25QH64A Based off of the now-abandoned GitHub pull request here: https://github.com/flashrom/flashrom/pull/239 This commit applies the changes on top of the refactor where the flash chip declarations were separated by vendor. Change-Id: I5b11e30f0a5357a6cbb32ddb93f450de5364c60b Co-authored-by: Ayushman Dutta <ayushman999(a)gmail.com> Co-authored-by: "aiyion.prime" <git(a)aiyionpri.me> Co-authored-by: Eric Park <me(a)ericswpark.com> Signed-off-by: Eric Park <me(a)ericswpark.com> --- M flashchips/xmc.c M include/flashchips.h 2 files changed, 42 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/90/86990/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/86990?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I5b11e30f0a5357a6cbb32ddb93f450de5364c60b Gerrit-Change-Number: 86990 Gerrit-PatchSet: 6 Gerrit-Owner: Eric Park <me(a)ericswpark.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Eric Park <me(a)ericswpark.com>

1 0

[S] Change in flashrom[main]: flashchips: Add XMC XM25QH64A
by Eric Park (Code Review) 25 Mar '25

25 Mar '25

Attention is currently required from: Eric Park, Nikolai Artemiev, Stefan Reinauer. Hello Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/86990?usp=email to look at the new patch set (#5). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: flashchips: Add XMC XM25QH64A ...................................................................... flashchips: Add XMC XM25QH64A Based off of the now-abandoned GitHub pull request here: https://github.com/flashrom/flashrom/pull/239 This commit applies the changes on top of the refactor where the flash chip declarations were separated by vendor. Change-Id: I5b11e30f0a5357a6cbb32ddb93f450de5364c60b Co-authored-by: Ayushman Dutta <ayushman999(a)gmail.com> Co-authored-by: "aiyion.prime" <git(a)aiyionpri.me> Co-authored-by: Eric Park <me(a)ericswpark.com> Signed-off-by: Eric Park <me(a)ericswpark.com> --- M flashchips/xmc.c M include/flashchips.h 2 files changed, 42 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/90/86990/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/86990?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I5b11e30f0a5357a6cbb32ddb93f450de5364c60b Gerrit-Change-Number: 86990 Gerrit-PatchSet: 5 Gerrit-Owner: Eric Park <me(a)ericswpark.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Eric Park <me(a)ericswpark.com>

1 0

[M] Change in flashrom[main]: flashchips: Add XMC25QH64A
by Anastasia Klimchuk (Code Review) 25 Mar '25

25 Mar '25

Attention is currently required from: Eric Park, Nikolai Artemiev, Stefan Reinauer. Anastasia Klimchuk has posted comments on this change by Eric Park. ( https://review.coreboot.org/c/flashrom/+/86990?usp=email ) Change subject: flashchips: Add XMC25QH64A ...................................................................... Patch Set 4: (5 comments) Patchset: PS4: Eric, thank you for the patch! I added a few comments. Also wanted to ask , have you tested yourself? I can see that PR was tested, although that was a few years ago. Commit Message: https://review.coreboot.org/c/flashrom/+/86990/comment/70dd1c56_53a08c69?us… : PS4, Line 7: XMC25QH64A Let's be exact so that it's searchable, call it `XMC XM25QH64A` https://review.coreboot.org/c/flashrom/+/86990/comment/fb56adb8_b1f3a6e2?us… : PS4, Line 18: Signed-off-by: Eric Park <me(a)ericswpark.com> Add yourself too as Co-authored-by (keep the sign off line too) File flashchips/xmc.c: https://review.coreboot.org/c/flashrom/+/86990/comment/065b46b7_a2863d6a?us… : PS4, Line 251: .name = "XM25QH64A", I would put this entry just before XM25QH128A in this file too (same as their ids are ordered), would you do this? thank you! File include/flashchips.h: https://review.coreboot.org/c/flashrom/+/86990/comment/42eb61fc_6c9cd9a4?us… : PS4, Line 1085: #define XMC_ID 0x20 /* same as ST_ID */ : #define XMC_XM25QH256B 0x6019 : #define XMC_XM25QH128B 0x6018 : #define XMC_XM25QH64A 0x7017 : #define XMC_XM25QH64B 0x6017 : #define XMC_XM25QH32B 0x4016 /* 0x6016 for QPI mode */ : #define XMC_XM25QH16B 0x4015 : #define XMC_XM25QH80B 0x4014 : #define XMC_XM25QH40B 0x4013 : #define XMC_XM25QU256B 0x7019 : #define XMC_XM25QU128B 0x5018 : #define XMC_XM25QU64A 0x3817 : #define XMC_XM25QU64B 0x5017 : #define XMC_XM25QU40B 0x5013 : #define XMC_XM25QU20B 0x5012 I know it was like this in the original pull request, but we don't need to do the aliasing. So you need to add only one line, for the device you are adding, this line: #define XMC_XM25QH64A 0x7017 insert it just before the existing macro for XMC_XM25QH128A with ID 0x7018 The rest of this block just remove. -- To view, visit https://review.coreboot.org/c/flashrom/+/86990?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: I5b11e30f0a5357a6cbb32ddb93f450de5364c60b Gerrit-Change-Number: 86990 Gerrit-PatchSet: 4 Gerrit-Owner: Eric Park <me(a)ericswpark.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Eric Park <me(a)ericswpark.com> Gerrit-Comment-Date: Tue, 25 Mar 2025 08:54:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[M] Change in flashrom[main]: libflashrom: Update the API for Logger Callback
by Anastasia Klimchuk (Code Review) 25 Mar '25

25 Mar '25

Attention is currently required from: Dmitry Zhadinets. Anastasia Klimchuk has posted comments on this change by Dmitry Zhadinets. ( https://review.coreboot.org/c/flashrom/+/86875?usp=email ) Change subject: libflashrom: Update the API for Logger Callback ...................................................................... Patch Set 6: (4 comments) Patchset: PS6: I have two small readability comments, otherwise the only question to decide is message size right? File include/libflashrom.h: https://review.coreboot.org/c/flashrom/+/86875/comment/475b5a75_6e689bb1?us… : PS4, Line 83: * > Hmmmm. The user does not affect this, but as you wish. I was trying to find it, but I couldn't, but then I found the addition at `flashrom_set_progress_callback_v2` :) I was thinking to add it at `flashrom_set_log_callback_v2` (the new fn that you are adding) File libflashrom.c: https://review.coreboot.org/c/flashrom/+/86875/comment/ad8d6a36_76bdf906?us… : PS4, Line 59: /* Dynamic messages and double formatting may not be ideal, : as they are slow. It would be better to limit the message : size to a fixed value (e.g., char message[1024] = {0};). */ > I would go shorter to perhaps 128 or 256 bytes, but have a fallback to dynamic allocation in case a […] I found one very long message in cli_classic.c, in `cli_classic_usage` (it prints help, as of `flashrom -h`), but I don't think it trigger a callback, since it's using printf directly? This was the only very long message that I found. The rest probably fits into 256, but maybe let's do it 512 just in case? Also 100% agree let's not drop long messages silently. Actually if we don't drop than 256 is fine File libflashrom.c: https://review.coreboot.org/c/flashrom/+/86875/comment/f4f4b00f_02493a53?us… : PS6, Line 84: if (!log_callback && global_log_callback == format_message_and_invoke_log_callback_v2) : global_log_callback = NULL; I spent some time looking at this, then I figured this is doing a reset (when log_callback is passed NULL). It is possible to understand, but I have a suggestion to change this a bit, to make it more readable (more obvious). What if you check if log_callback is NULL, and then assign all 3 values to NULL? and with 1-line comment like "Reset all callbacks since NULL is passed" I know this code works already, my comment is only about readability. ``` if (!log_callback) { /* Reset all callbacks because NULL is passed */ global_log_callback = NULL; global_log_callback_v2 = NULL; global_log_user_data = NULL; } else { .... and the rest as normal flow } ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/86875?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: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Gerrit-Change-Number: 86875 Gerrit-PatchSet: 6 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Comment-Date: Tue, 25 Mar 2025 07:10:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Dmitry Zhadinets <dzhadinets(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2025