December 2021 - flashrom-gerrit

Change in flashrom[master]: dummyflasher: add SR2 emulation harness
by Sergii Dmytruk (Code Review) 18 Dec '21

18 Dec '21

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Hello build bot (Jenkins), Nico Huber, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59072 to look at the new patch set (#18). Change subject: dummyflasher: add SR2 emulation harness ...................................................................... dummyflasher: add SR2 emulation harness Prepare everything for emulating SR2 for chips that have it. This is needed for accessing second SRP bit which is involved in write protection. The emulated register doesn't affect anything yet and will be tested by write-protection tests. Change-Id: I177ae3f068f03380f5b3941d9996a07205672e59 Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> --- M dummyflasher.c M flashrom.8.tmpl 2 files changed, 81 insertions(+), 15 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/72/59072/18 -- To view, visit https://review.coreboot.org/c/flashrom/+/59072 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I177ae3f068f03380f5b3941d9996a07205672e59 Gerrit-Change-Number: 59072 Gerrit-PatchSet: 18 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashchips: enable write-protection for W25Q{64,128}.V
by Sergii Dmytruk (Code Review) 18 Dec '21

18 Dec '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk, Sergii Dmytruk. Hello build bot (Jenkins), Nico Huber, Angel Pons, Nikolai Artemiev, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59071 to look at the new patch set (#16). Change subject: flashchips: enable write-protection for W25Q{64,128}.V ...................................................................... flashchips: enable write-protection for W25Q{64,128}.V Configuration for W25Q64 was tested on hardware (W25Q64FV). Emulation of W25Q128 in dummyflasher will be extended to support WP. Haven't tested this one on hardware, but it's the same configuration as for W25Q64. Change-Id: Iccb69a8d3a0dd2192e2c938caddaf07b1889ed35 Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> --- M flashchips.c 1 file changed, 22 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/71/59071/16 -- To view, visit https://review.coreboot.org/c/flashrom/+/59071 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iccb69a8d3a0dd2192e2c938caddaf07b1889ed35 Gerrit-Change-Number: 59071 Gerrit-PatchSet: 16 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: dummyflasher: emulate SR2 for W25Q128FV
by Sergii Dmytruk (Code Review) 18 Dec '21

18 Dec '21

Attention is currently required from: Nico Huber, Angel Pons. Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59073 ) Change subject: dummyflasher: emulate SR2 for W25Q128FV ...................................................................... Patch Set 17: (2 comments) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/59073/comment/0b4cf592_6f3cd61f PS16, Line 344: data->emu_status[1] |= (writearr[2] & ~ro_bits); > Spurious change? Ack https://review.coreboot.org/c/flashrom/+/59073/comment/73bb1ceb_c0993e60 PS16, Line 180: } > So, if we treat this as yet another property of the emulated chip, […] We can't generalize this. Exact value of the mask changes depending on the values of separate bits, which are set dynamically, so custom code will always be needed. -- To view, visit https://review.coreboot.org/c/flashrom/+/59073 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I79f9b4a0b604663d3288ad70dcbe3ea4075dede5 Gerrit-Change-Number: 59073 Gerrit-PatchSet: 17 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> 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: Sat, 18 Dec 2021 18:06:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dummyflasher: add SR2 emulation harness
by Sergii Dmytruk (Code Review) 18 Dec '21

18 Dec '21

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59072 ) Change subject: dummyflasher: add SR2 emulation harness ...................................................................... Patch Set 17: (11 comments) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/59072/comment/2456ce6a_ffa6b39e PS16, Line 42: int emu_features; /* combination of FEATURE_* constants */ > It seems a little odd to have this while most of the code doesn't make […] I thought it might be extended in the future to handle more `FEATURE_*` flags so emulation would match chip definitions more closely, but I guess that can be done when there will be multiple flags like that. https://review.coreboot.org/c/flashrom/+/59072/comment/e4f201ca_c55852cd PS16, Line 167: return (status_reg == STATUS1 ? SPI_SR_WEL | SPI_SR_WIP : 0); > Nit, no parentheses needed. Done. https://review.coreboot.org/c/flashrom/+/59072/comment/72b8c300_725311c9 PS16, Line 168: } > This seems very chip specific. This applies to all of the emulated chips, later commits add chip-specific things. https://review.coreboot.org/c/flashrom/+/59072/comment/60b08b94_9d437469 PS16, Line 335: wrsr_ext = (writecnt == 3 && (data->emu_features & FEATURE_WRSR_EXT)); > In this case, I think the additional parentheses make it actually harder […] Done https://review.coreboot.org/c/flashrom/+/59072/comment/9c02079c_cef805b6 PS16, Line 338: ro_bits = get_status_ro_bit_mask(STATUS1); > Looks like this changes behavior, i.e. fixes that the WEL could be […] It doesn't, WEL bit is reset at the bottom of the function. Marking it here as RO just makes its read-only status more explicit. https://review.coreboot.org/c/flashrom/+/59072/comment/c86f75c9_bbc38b3c PS16, Line 339: data->emu_status[0] &= ro_bits; > This also changes behavior, I guess WIP is not accidentally cleared anymore?. WIP was RO before, old code is: data->emu_status = writearr[1] & ~SPI_SR_WIP; https://review.coreboot.org/c/flashrom/+/59072/comment/50def676_6c486ad2 PS16, Line 340: data->emu_status[0] |= (writearr[1] & ~ro_bits); > Nit, no parentheses needed. Done https://review.coreboot.org/c/flashrom/+/59072/comment/670272cf_a45477d4 PS16, Line 347: * > No dangling asterisk, please. Done https://review.coreboot.org/c/flashrom/+/59072/comment/5f725598_b9fb61be PS16, Line 347: other chips might differ At least datasheet for W25Q128FV claims otherwise in its note on previous generations: > If /CS is driven high after the eighth clock, the Write Status > Register-1 (01h) instruction will only program the Status Register-1, > the Status Register-2 will not be affected (Previous generations will > clear CMP and QE bits). https://review.coreboot.org/c/flashrom/+/59072/comment/35c1c69a_bfb4daf0 PS16, Line 364: data->emu_status[1] |= (writearr[1] & ~ro_bits); > Should we add a debug message like above? Done https://review.coreboot.org/c/flashrom/+/59072/comment/0a7263bc_5a4e031e PS16, Line 970: 32 > Why 32? generally, it seems odd to give an exact number when no exact […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/59072 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I177ae3f068f03380f5b3941d9996a07205672e59 Gerrit-Change-Number: 59072 Gerrit-PatchSet: 17 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sat, 18 Dec 2021 18:03:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC][OTP] dummyflasher: add EN25QH128 chip with OTP mode
by Sergii Dmytruk (Code Review) 18 Dec '21

18 Dec '21

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Hello build bot (Jenkins), Nico Huber, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59708 to look at the new patch set (#9). Change subject: [RFC][OTP] dummyflasher: add EN25QH128 chip with OTP mode ...................................................................... [RFC][OTP] dummyflasher: add EN25QH128 chip with OTP mode This chip demonstrates seecond kind of OTP: mode upon entering which regular read/write/erase commands can be used to interact with OTP content. It will also be used in tests. Change-Id: Id5857db43ebf2613bdb5e99342d28d4e0981a6b8 Signed-off-by: Hatim Kanchwala <hatim at hatimak.me> Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> --- M dummyflasher.c M flashrom.8.tmpl 2 files changed, 97 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/59708/9 -- To view, visit https://review.coreboot.org/c/flashrom/+/59708 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id5857db43ebf2613bdb5e99342d28d4e0981a6b8 Gerrit-Change-Number: 59708 Gerrit-PatchSet: 9 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: dummyflasher: enforce write protection for W25Q128FV
by Sergii Dmytruk (Code Review) 18 Dec '21

18 Dec '21

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Hello build bot (Jenkins), Nico Huber, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59074 to look at the new patch set (#17). Change subject: dummyflasher: enforce write protection for W25Q128FV ...................................................................... dummyflasher: enforce write protection for W25Q128FV Start taking bits related to write protection into account. Also add "hwwp" parameter for dummy programmer that sets state of WP pin (not inverted value). dummyflasher doesn't store state of the chip between runs and flashrom allows running only one command, so testing WP in this way is limited. However, WP options can be combined with other operations and are executed prior to them, so certain scenarios can be checked. List possible ranges: flashrom -p dummy:emulate=W25Q128FV,hwwp=yes --wp-list Set a particular range and check status is correct: flashrom -p dummy:emulate=W25Q128FV,hwwp=yes \ --wp-enable \ --wp-range=0x00100000,0x00f00000 \ --wp-status Enable write protection and try erasing/writing (erasing here): # this fails flashrom -p dummy:emulate=W25Q128FV,hwwp=yes \ --wp-range=0,0x00c00000 \ --wp-enable \ --erase Write protecting empty range has no effect: # this succeeds flashrom -p dummy:emulate=W25Q128FV,hwwp=yes \ --wp-range=0,0 \ --wp-enable \ --erase Change-Id: I9fd1417f941186391bd213bd355530143c8f04a0 Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> --- M dummyflasher.c M flashrom.8.tmpl 2 files changed, 155 insertions(+), 18 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/59074/17 -- To view, visit https://review.coreboot.org/c/flashrom/+/59074 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9fd1417f941186391bd213bd355530143c8f04a0 Gerrit-Change-Number: 59074 Gerrit-PatchSet: 17 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: dummyflasher: emulate SR2 for W25Q128FV
by Sergii Dmytruk (Code Review) 18 Dec '21

18 Dec '21

Attention is currently required from: Angel Pons, Sergii Dmytruk. Hello build bot (Jenkins), Nico Huber, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59073 to look at the new patch set (#17). Change subject: dummyflasher: emulate SR2 for W25Q128FV ...................................................................... dummyflasher: emulate SR2 for W25Q128FV Enable emulation of SR2 for W25Q128FV and provide logic for updating it (mask of read-only bits that can't be set from outside). W25Q128FV has three status registers, but no plans to use the third one yet. Change-Id: I79f9b4a0b604663d3288ad70dcbe3ea4075dede5 Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> --- M dummyflasher.c 1 file changed, 22 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/73/59073/17 -- To view, visit https://review.coreboot.org/c/flashrom/+/59073 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I79f9b4a0b604663d3288ad70dcbe3ea4075dede5 Gerrit-Change-Number: 59073 Gerrit-PatchSet: 17 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: dummyflasher: add SR2 emulation harness
by Sergii Dmytruk (Code Review) 18 Dec '21

18 Dec '21

Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Hello build bot (Jenkins), Nico Huber, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59072 to look at the new patch set (#17). Change subject: dummyflasher: add SR2 emulation harness ...................................................................... dummyflasher: add SR2 emulation harness Prepare everything for emulating SR2 for chips that have it. This is needed for accessing second SRP bit which is involved in write protection. The emulated register doesn't affect anything yet and will be tested by write-protection tests. Change-Id: I177ae3f068f03380f5b3941d9996a07205672e59 Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> --- M dummyflasher.c M flashrom.8.tmpl 2 files changed, 81 insertions(+), 15 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/72/59072/17 -- To view, visit https://review.coreboot.org/c/flashrom/+/59072 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I177ae3f068f03380f5b3941d9996a07205672e59 Gerrit-Change-Number: 59072 Gerrit-PatchSet: 17 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: dummyflasher: emulate SR2 for W25Q128FV
by Nico Huber (Code Review) 18 Dec '21

18 Dec '21

Attention is currently required from: Angel Pons, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59073 ) Change subject: dummyflasher: emulate SR2 for W25Q128FV ...................................................................... Patch Set 16: (2 comments) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/59073/comment/67ffe4e9_6ef0660c PS16, Line 344: data->emu_status[1] |= (writearr[2] & ~ro_bits); Spurious change? https://review.coreboot.org/c/flashrom/+/59073/comment/2251c085_02399f3d PS16, Line 180: } So, if we treat this as yet another property of the emulated chip, wouldn't it make sense to add the mask(s) to `struct emu_data`? (and fill it when the chip is selected, like all the other properties) -- To view, visit https://review.coreboot.org/c/flashrom/+/59073 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I79f9b4a0b604663d3288ad70dcbe3ea4075dede5 Gerrit-Change-Number: 59073 Gerrit-PatchSet: 16 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 18 Dec 2021 14:35:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dummyflasher: add SR2 emulation harness
by Nico Huber (Code Review) 18 Dec '21

18 Dec '21

Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59072 ) Change subject: dummyflasher: add SR2 emulation harness ...................................................................... Patch Set 16: Code-Review+1 (12 comments) Patchset: PS16: Many thanks for extending the dummy programmer! There is one thing that your patch actually seems to fix. Please always push such fixes in separate commits (ahead or even in a separate queue so they can be merged easily). Looks quite good otherwise. File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/59072/comment/a42040d7_56bdc91b PS16, Line 42: int emu_features; /* combination of FEATURE_* constants */ It seems a little odd to have this while most of the code doesn't make use of it. If you are going to check only one (or a few) bits, why not just add individual `bool` flags? That would even be more readable, I guess. https://review.coreboot.org/c/flashrom/+/59072/comment/487baa09_78633c0e PS16, Line 167: return (status_reg == STATUS1 ? SPI_SR_WEL | SPI_SR_WIP : 0); Nit, no parentheses needed. https://review.coreboot.org/c/flashrom/+/59072/comment/05ba2f37_4b3f2d50 PS16, Line 168: } This seems very chip specific. https://review.coreboot.org/c/flashrom/+/59072/comment/5ee23b72_8eeae367 PS16, Line 335: wrsr_ext = (writecnt == 3 && (data->emu_features & FEATURE_WRSR_EXT)); In this case, I think the additional parentheses make it actually harder to read the line. For every open parenthesis, one has to keep track where it's closed again. https://review.coreboot.org/c/flashrom/+/59072/comment/dccc2f50_a1ed8d1e PS16, Line 338: ro_bits = get_status_ro_bit_mask(STATUS1); Looks like this changes behavior, i.e. fixes that the WEL could be written before? Please put such changes into separate commits. https://review.coreboot.org/c/flashrom/+/59072/comment/a1022284_86574c25 PS16, Line 339: data->emu_status[0] &= ro_bits; This also changes behavior, I guess WIP is not accidentally cleared anymore?. https://review.coreboot.org/c/flashrom/+/59072/comment/56cb96c8_62856af0 PS16, Line 340: data->emu_status[0] |= (writearr[1] & ~ro_bits); Nit, no parentheses needed. https://review.coreboot.org/c/flashrom/+/59072/comment/2f689e6a_370a7491 PS16, Line 347: other chips might differ Isn't this always the case? https://review.coreboot.org/c/flashrom/+/59072/comment/a9f606e8_9727776c PS16, Line 347: * No dangling asterisk, please. https://review.coreboot.org/c/flashrom/+/59072/comment/9337ac17_a245db49 PS16, Line 364: data->emu_status[1] |= (writearr[1] & ~ro_bits); Should we add a debug message like above? https://review.coreboot.org/c/flashrom/+/59072/comment/a2e58c01_78041f10 PS16, Line 970: 32 Why 32? generally, it seems odd to give an exact number when no exact number is needed, i.e. `unsigned int` would also do. -- To view, visit https://review.coreboot.org/c/flashrom/+/59072 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I177ae3f068f03380f5b3941d9996a07205672e59 Gerrit-Change-Number: 59072 Gerrit-PatchSet: 16 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 18 Dec 2021 14:28:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2021