January 2022 - flashrom-gerrit

Change in flashrom[master]: SFDP: drop static table length check to make newer SFDP revisions work
by Anastasia Klimchuk (Code Review) 16 Jan '22

16 Jan '22

Attention is currently required from: Edward O'Callaghan, Felix Singer, Nico Huber, Angel Pons, Michael Niewöhner. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60055 ) Change subject: SFDP: drop static table length check to make newer SFDP revisions work ...................................................................... Patch Set 2: (1 comment) File sfdp.c: https://review.coreboot.org/c/flashrom/+/60055/comment/ae3662b7_d27222df PS2, Line 133: msg_cdbg("Parsing JEDEC flash parameter table... "); : msg_cdbg2("\n"); > The line break is only wanted if the next message is at level dbg2 too. The next message is inside `switch (tmp8)`? Then it means informational messages are printed on the next line, and error messages are printed on the same line. I don't fully understand why, but if this is by design - all good! -- To view, visit https://review.coreboot.org/c/flashrom/+/60055 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id84cde4ebc805d68e2984e8041fbc48d7ceebe34 Gerrit-Change-Number: 60055 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Sun, 16 Jan 2022 22:35:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: SFDP: drop static table length check to make newer SFDP revisions work
by Michael Niewöhner (Code Review) 16 Jan '22

16 Jan '22

Attention is currently required from: Edward O'Callaghan, Felix Singer, Nico Huber, Angel Pons, Anastasia Klimchuk. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60055 ) Change subject: SFDP: drop static table length check to make newer SFDP revisions work ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/60055/comment/e4a08f25_5f51fcba PS1, Line 28: ***RFC*** We could maintain a list of revisions/lengths but is it really : worth it? : > and that the major version is 1. That's already checked. But..... According to JESD216 there is a conflict between JESD216 and Intel legacy. It reads like Intel used 1.x, x=[1-4] \o/ I couldn't find *anything* about that, though....... -- To view, visit https://review.coreboot.org/c/flashrom/+/60055 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id84cde4ebc805d68e2984e8041fbc48d7ceebe34 Gerrit-Change-Number: 60055 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> 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: Sun, 16 Jan 2022 18:38:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C
by Michael Niewöhner (Code Review) 16 Jan '22

16 Jan '22

Attention is currently required from: Nikolai Artemiev. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52883 ) Change subject: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C ...................................................................... Patch Set 2: (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/52883/comment/a9acd33c_32011522 PS2, Line 6585: GD25Q127C > > And there I thought they put the Q into the model number for QPI... have you […] Ohhh, check out page 13! -- To view, visit https://review.coreboot.org/c/flashrom/+/52883 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I688f2e15aef61afbec728a9a81094bee56d6fbfa Gerrit-Change-Number: 52883 Gerrit-PatchSet: 2 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 16 Jan 2022 18:35:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C
by Michael Niewöhner (Code Review) 16 Jan '22

16 Jan '22

Attention is currently required from: Nikolai Artemiev. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52883 ) Change subject: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C ...................................................................... Patch Set 2: (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/52883/comment/5f8b984b_3418efbb PS2, Line 6585: GD25Q127C > And there I thought they put the Q into the model number for QPI... have you tested it? maybe the datasheet is just wrong. I have no QPI interface, so no, not tested. According to [1] Q/B/M stand for 3.3V + Quad I/O [1] https://www.endrich.com/sixcms/media.php/2308/GigaDevice%20Product%20Select… -- To view, visit https://review.coreboot.org/c/flashrom/+/52883 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I688f2e15aef61afbec728a9a81094bee56d6fbfa Gerrit-Change-Number: 52883 Gerrit-PatchSet: 2 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 16 Jan 2022 18:33:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: SFDP: drop static table length check to make newer SFDP revisions work
by Nico Huber (Code Review) 16 Jan '22

16 Jan '22

Attention is currently required from: Edward O'Callaghan, Felix Singer, Angel Pons, Michael Niewöhner, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60055 ) Change subject: SFDP: drop static table length check to make newer SFDP revisions work ...................................................................... Patch Set 2: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/60055/comment/82ec5a45_539cb5e4 PS1, Line 28: ***RFC*** We could maintain a list of revisions/lengths but is it really : worth it? : > Good question. I think there are three cases: […] Sounds much like we'd want to check that the length is at least that of 1.0 (or more specifically the minimum length that covers the last parameter that flashrom consumes) and that the major version is 1. File sfdp.c: https://review.coreboot.org/c/flashrom/+/60055/comment/84654f46_05025282 PS2, Line 133: msg_cdbg("Parsing JEDEC flash parameter table... "); : msg_cdbg2("\n"); > Looks like these two message can be merged into one now. […] The line break is only wanted if the next message is at level dbg2 too. -- To view, visit https://review.coreboot.org/c/flashrom/+/60055 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id84cde4ebc805d68e2984e8041fbc48d7ceebe34 Gerrit-Change-Number: 60055 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 16 Jan 2022 17:17:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C
by Nico Huber (Code Review) 16 Jan '22

16 Jan '22

Attention is currently required from: Nikolai Artemiev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52883 ) Change subject: flashchips.c: merge GD25B128B/GD25Q128B and GD25Q127C/GD25Q128C ...................................................................... Patch Set 2: (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/52883/comment/13148054_2e3aa6bd PS2, Line 6585: GD25Q127C > this one is already wrong, since there is no QPI support. […] And there I thought they put the Q into the model number for QPI... have you tested it? maybe the datasheet is just wrong. -- To view, visit https://review.coreboot.org/c/flashrom/+/52883 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I688f2e15aef61afbec728a9a81094bee56d6fbfa Gerrit-Change-Number: 52883 Gerrit-PatchSet: 2 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 16 Jan 2022 16:55:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add functions for reading/writing WP settings
by Nico Huber (Code Review) 16 Jan '22

16 Jan '22

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev, Anastasia Klimchuk, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 ) Change subject: libflashrom,writeprotect: add functions for reading/writing WP settings ...................................................................... Patch Set 27: (6 comments) Patchset: PS22: > Passing an abstract configuration structure to WP functions is probably a better option than the int […] Sorry for the silence. I think you made good progress on your own! The abstract struct leaves the function signatures nice and simple. However, exposed structs form an ABI along with potential trouble (see inline comment). It might be better to have an opaque struct with getters and setters, e.g. ``` enum fashrom_wp_mode { WP_MODE_DISABLE, WP_MODE_HARDWARE, }; struct flashrom_wp_cfg; int flashrom_wp_cfg_new(struct flashrom_wp_cfg **); void flashrom_wp_cfg_release(struct flashrom_wp_cfg *); void flashrom_wp_set_mode(struct flashrom_wp_cfg *, enum flashrom_wp_mode); enum flashrom_wp_mode flashrom_wp_get_mode(const struct flashrom_wp_cfg *); void flashrom_wp_set_range(struct flashrom_wp_cfg *, size_t start, size_t len); void flashrom_wp_get_range(size_t *start, size_t *len, const struct flashrom_wp_cfg *); int flashrom_wp_get_cfg(struct flashrom_wp_cfg **, struct flashrom_flashctx *); int flashrom_wp_set_cfg(struct flashrom_flashctx *, const struct flashrom_wp_cfg *); ``` This would allow us to extend the API in the future if necessary without touching any of the lines above. WDYT? File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58479/comment/32b00ac6_d4caf998 PS27, Line 133: }; Having a struct in the API means we can't ever change the struct without versioning the ABI. Beside that I don't know the mechanisms behind the latter I think it's not necessary. For instance, we could have an opaque object like the others, and getters and setters for mode and range, WDYT? https://review.coreboot.org/c/flashrom/+/58479/comment/af7b568a_0fab4e44 PS27, Line 138: #define FLASHROM_WP_ERR_VERIFY_FAILED 4 This could be an enum, maybe even without specific numbers? File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58479/comment/7243012f_9f680caa PS22, Line 63: *cfg = (struct wp_chip_config) {0}; > I think I might have relied on this in an earlier patch set but I don't remember now. […] Done File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58479/comment/d270cf6d_1b5994c8 PS27, Line 118: } Maybe drop braces here for concistency. https://review.coreboot.org/c/flashrom/+/58479/comment/8415658b_aa245944 PS27, Line 161: to write : * to the chip drop -- To view, visit https://review.coreboot.org/c/flashrom/+/58479 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad Gerrit-Change-Number: 58479 Gerrit-PatchSet: 27 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sun, 16 Jan 2022 01:25:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: writeprotect.h: add structure to represent chip wp configuration bits
by Nico Huber (Code Review) 15 Jan '22

15 Jan '22

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/+/58478 ) Change subject: writeprotect.h: add structure to represent chip wp configuration bits ...................................................................... Patch Set 26: Code-Review+1 (2 comments) File flash.h: https://review.coreboot.org/c/flashrom/+/58478/comment/73759344_e228f8dc PS22, Line 201: Complete > The intention is that it will be kept complete, i.e. […] Your intentions don't change that the comment is misleading. If you need to explain a comment what's that? a meta comment? And even according to what you describe ("as necessary to support new chips") implies that we'd support all WP features of a chip, which we don't even for the first added chips AFAICS. You could say it's complete wrt. the block protection configuration and then hope the comment stays true for added chips. Generally, code comments are dangerous. People often trust comments instead of reading the code. If a comment is wrong, misleading or even just outdated, that's very bad for maintenance. What makes things worse is that developers and reviewers often ignore existing comments when the code changes. Especially when comments state something that was obvious to them in the first place. Better not write comments that are prone to misinterpretation or to get outdated. Actually, I think you already found better words yourself :) It allows most WP code to store and manipulate a chip's configuration without knowing the exact layout of bits in the chip's status registers. That's something that explain the idea of the struct, I'd simply leave out the completeness claim and add the above: /* * Description of a chip's write protection configuration * * It allows most WP code to store and manipulate a chip's configuration * without knowing the exact layout of bits in the chip's status registers. */ (NB. Please don't mark threads as resolved unless you are 100% sure a consent has been found.) https://review.coreboot.org/c/flashrom/+/58478/comment/d47bba1b_d4eb302d PS22, Line 202: chip > I've changed it to `wp_bits`. […] Thanks! -- To view, visit https://review.coreboot.org/c/flashrom/+/58478 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17dee630248ce7b51e624a6e46d7097d5d0de809 Gerrit-Change-Number: 58478 Gerrit-PatchSet: 26 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> 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, 15 Jan 2022 17:15:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips: add writeprotect bit layout map to chips
by Nico Huber (Code Review) 15 Jan '22

15 Jan '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Anastasia Klimchuk, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58477 ) Change subject: flashchips: add writeprotect bit layout map to chips ...................................................................... Patch Set 25: (3 comments) File flash.h: https://review.coreboot.org/c/flashrom/+/58477/comment/c7f6bc75_a2b8ae7f PS25, Line 287: struct reg_bit_info cmp; Please add a comment about the acronyms. (Thought I mentioned that before *shrug* but can't find the comment.) File flash.h: https://review.coreboot.org/c/flashrom/+/58477/comment/14f7ac77_64c88f6e PS21, Line 192: 0 > Done Still, why the 0? File flashchips.c: https://review.coreboot.org/c/flashrom/+/58477/comment/e9c09bdb_59a464d0 PS20, Line 6353: .tb = {STATUS1, 5, RW}, /* Called BP3 in datasheet, acts like TB */ : .sec = {STATUS1, 6, RW}, /* Called BP4 in datasheet, acts like SEC */ > Any thoughts about how this should be handled? […] IMO we should definitely use the naming by the bit's function, like you did. To be visible to new developers, we could also add comments to the struct declaration, e.g. "Some datasheets call it BP3 even if it acts as TB. */ There's some documentation in the wiki, would be nice to update it and also mention this problem. https://flashrom.org/Development_Guidelines#Adding/reviewing_a_new_flash_ch… -- To view, visit https://review.coreboot.org/c/flashrom/+/58477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id08d77e6d4ca5109c0d698271146d026dbc21284 Gerrit-Change-Number: 58477 Gerrit-PatchSet: 25 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 15 Jan 2022 16:05:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg,flashchips: add SR2 read/write support
by Nico Huber (Code Review) 15 Jan '22

15 Jan '22

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/+/58570 ) Change subject: spi25_statusreg,flashchips: add SR2 read/write support ...................................................................... Patch Set 22: Code-Review+1 (3 comments) File flash.h: https://review.coreboot.org/c/flashrom/+/58570/comment/dd5ef876_7b9908ed PS22, Line 145: #define FEATURE_RDSR2 (1 << 21) Isn't this implied by the above and function pointers that make use of it? If it is redundant, it only creates more things to check for in the code. https://review.coreboot.org/c/flashrom/+/58570/comment/7504d340_cd9a710a PS22, Line 147: Only one empty line please. File flashchips.c: https://review.coreboot.org/c/flashrom/+/58570/comment/e90f6525_d12b5887 PS18, Line 6711: FEATURE_WRSR_EXT > At least according to datasheet rev 2. […] Interesting, looks like the datasheet is inconsistent. It first states: "CS# must be driven high after the eighth of the data Byte has been latched in. If not, the Write Status Register (WRSR) command is not executed." Later it says one could write SR1 and SR2 with 01h... I'd prefer to have it tested. And a comment about the datasheet oddity would be nice. -- To view, visit https://review.coreboot.org/c/flashrom/+/58570 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I34a503b0958e8f2f22a2a993a6ea529eb46b41db Gerrit-Change-Number: 58570 Gerrit-PatchSet: 22 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> 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, 15 Jan 2022 15:15:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit January 2022