December 2021 - flashrom-gerrit

Change in flashrom[master]: [RFC][WPTST] dummyflasher: SR2 for W25Q128FV
by Anastasia Klimchuk (Code Review) Dec. 1, 2021

Dec. 1, 2021

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59073 ) Change subject: [RFC][WPTST] dummyflasher: SR2 for W25Q128FV ...................................................................... Patch Set 10: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/59073/comment/4a6f2b8a_d2d91567 PS10, Line 8: : By the way, W25Q128FV has three status registers, but no plans to use : the third one yet. If you remove "by the way", it is a very good second paragraph for this commit message! The first paragraph needs to describe more precisely what this commit is doing. A wider question is about this and previous patch, what exactly each of them is doing. Maybe the work between them can be re-distributed (for example, previous patch has branch with emu_status_len == 2 , but the value set to 2 only here). I am saying "maybe" because it is a question, let's look closely. After the work is distributed between two patches, then commit description and commit message for each of them need to reflect that. At the moment commit descriptions are: "support emulation of SR2" "SR2 for W25Q128FV" Imagine looking at commit history a year after. It would not be easy to guess what each patch is doing. File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/59073/comment/70006fff_8952b246 PS10, Line 922: data->emu_status_len = 2; I added a comment to previous patch about emu_status_len always 1, I see that it becomes 2 in this patch. However, in the previous patch it is always 1. Maybe some of the parts of previous patch should be here? -- 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: 10 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: Anastasia Klimchuk <aklm(a)chromium.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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 01:16:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC][WPTST] dummyflasher: support emulation of SR2
by Anastasia Klimchuk (Code Review) Dec. 1, 2021

Dec. 1, 2021

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59072 ) Change subject: [RFC][WPTST] dummyflasher: support emulation of SR2 ...................................................................... Patch Set 10: (5 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/59072/comment/a0deed55_d3e11612 PS10, Line 11: You've probably tested this patch, probably by running dummyflasher with some programmer params. It would be great to add this info into a commit message. File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/59072/comment/cfcbe318_025668d1 PS10, Line 47: uint8_t emu_status_len; /* number of emulated status bytes */ Is this a number of emulated status bytes in SR2? https://review.coreboot.org/c/flashrom/+/59072/comment/d74b80c2_ab9d30aa PS10, Line 342: /* XXX: Else reset SR2 parts to zero? */ What does it mean? Is it a TODO? https://review.coreboot.org/c/flashrom/+/59072/comment/b14ba74c_e312078c PS10, Line 930: data->emu_status_len = 1; Maybe I missed something, but is this value always initialised to 1? File flashrom.8.tmpl: https://review.coreboot.org/c/flashrom/+/59072/comment/cb05a268_cd367d21 PS10, Line 781: is an 8-bit or 16-bit hexadecimal value. What does this value mean? I know it was like this before ;) But since you are changing/upgrading this programmer param, you probably know what it is and can add some explanation here. Ideally also examples of 8-bit and 16-bit spi_status values. -- 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: 10 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: Anastasia Klimchuk <aklm(a)chromium.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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 00:47:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

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

Dec. 1, 2021

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59071 ) Change subject: [RFC][WPTST] flashchips: enable write-protection for W25Q{64,128}.V ...................................................................... Patch Set 10: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/59071/comment/a217a007_83ae04b8 PS10, Line 7: enable write-protection for W25Q{64,128}.V > I thought these are real chips, not only "mock chips used for emulation"? It just so happens that du […] Yes, these are real chips. I've tested code on a W25Q64 chip, W25Q128V has absolutely identical configuration per datasheets. If code is known to work in general, I'm not sure every individual configuration of bits needs to be tested on hardware before enabling the support (and in case of OTP this is even harder). -- 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: 10 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: Anastasia Klimchuk <aklm(a)chromium.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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 01 Dec 2021 00:36:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC][WPTST] flashchips: enable write-protection for W25Q{64,128}.V
by Anastasia Klimchuk (Code Review) Dec. 1, 2021

Dec. 1, 2021

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59071 ) Change subject: [RFC][WPTST] flashchips: enable write-protection for W25Q{64,128}.V ...................................................................... Patch Set 10: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/59071/comment/28b496d0_fd983d75 PS10, Line 7: enable write-protection for W25Q{64,128}.V I thought these are real chips, not only "mock chips used for emulation"? It just so happens that dummy can emulate one of them. If what I think is true and these are real chips, then enabling wp on them needs to be tested? -- 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: 10 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: Anastasia Klimchuk <aklm(a)chromium.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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 01 Dec 2021 00:19:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0