Attention is currently required from: Nico Huber, Paul Menzel.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60342 )
Change subject: hwaccess_x86_msr: fix build for FreeBSD
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/60342/comment/0ac82dfb_72cf593e
PS1, Line 7: hwaccess_x86_msr: buildfix for FreeBSD
> Nit: Make it a statement: Fix build for FreeBSD
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/60342
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2045345878392436b0ea4d6bd4f2896edc645673
Gerrit-Change-Number: 60342
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Idwer Vollering <vidwer(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Fri, 24 Dec 2021 18:16:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Thomas Heijligen.
Hello build bot (Jenkins), Nico Huber, Idwer Vollering, Paul Menzel, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/60342
to look at the new patch set (#2).
Change subject: hwaccess_x86_msr: fix build for FreeBSD
......................................................................
hwaccess_x86_msr: fix build for FreeBSD
Add missing includes for FreeBSD
Change-Id: I2045345878392436b0ea4d6bd4f2896edc645673
Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com>
---
M hwaccess_x86_msr.c
1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/60342/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/60342
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2045345878392436b0ea4d6bd4f2896edc645673
Gerrit-Change-Number: 60342
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Idwer Vollering <vidwer(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Thomas Heijligen.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60342 )
Change subject: hwaccess_x86_msr: buildfix for FreeBSD
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/60342/comment/e05551a2_e29719c5
PS1, Line 7: hwaccess_x86_msr: buildfix for FreeBSD
Nit: Make it a statement: Fix build for FreeBSD
--
To view, visit https://review.coreboot.org/c/flashrom/+/60342
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2045345878392436b0ea4d6bd4f2896edc645673
Gerrit-Change-Number: 60342
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Idwer Vollering <vidwer(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Fri, 24 Dec 2021 16:20:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen.
Idwer Vollering has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60342 )
Change subject: hwaccess_x86_msr: buildfix for FreeBSD
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/60342
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2045345878392436b0ea4d6bd4f2896edc645673
Gerrit-Change-Number: 60342
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Idwer Vollering <vidwer(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Fri, 24 Dec 2021 14:06:05 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Sergii Dmytruk.
Nikolai Artemiev 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 22:
(2 comments)
File flash.h:
https://review.coreboot.org/c/flashrom/+/58477/comment/3226bb7c_aac9d49a
PS21, Line 192: 0
> Why give these numbers? Is there anything to sync with?
Done
https://review.coreboot.org/c/flashrom/+/58477/comment/f4d565bb_4371dc16
PS21, Line 283: */
> We should generally try to find a better representation for the flash chips. […]
That's a good point, thanks.
--
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: 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: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Fri, 24 Dec 2021 06:48:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Sergii Dmytruk.
Nikolai Artemiev 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 19:
(3 comments)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/58570/comment/182e9933_af57a8c5
PS18, Line 6711: FEATURE_WRSR_EXT
> The datasheet suggests this won't work, AFAICT, and I guess this path […]
At least according to datasheet rev 2.1 the WRSR (01h) command can write both registers together, I don't remember whether I tested it though.
https://review.coreboot.org/c/flashrom/+/58570/comment/13704bea_0dd976f1
PS18, Line 6872: .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP,
> Spurious change?
Done
File spi25_statusreg.c:
https://review.coreboot.org/c/flashrom/+/58570/comment/c95fca3a_704e398c
PS17, Line 109: if (spi_read_register(flash, STATUS1, &sr1))
: return 1;
> Would it make sense to print an error here?
Done
--
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: 19
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: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Fri, 24 Dec 2021 06:48:21 +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>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58475 )
Change subject: spi25_statusreg: make register read/write functions generic
......................................................................
Patch Set 16:
(12 comments)
File flash.h:
https://review.coreboot.org/c/flashrom/+/58475/comment/23162502_2572ba7a
PS15, Line 170: INVALID_REG = 0,
: STATUS1,
: STATUS2,
: CONFIG1,
> Please add entries in the commits that make use of them. Took me rather […]
Done. Yes, I had CONFIG1 for some Macronix chips, but I'll add it back later if it's needed.
File flash.h:
https://review.coreboot.org/c/flashrom/+/58475/comment/fcf847f4_b2c282fd
PS14, Line 169: enum flash_reg {
: INVALID_REG = 0,
: STATUS1,
: STATUS2,
: CONFIG1,
: };
: #define MAX_REGISTERS 4
> `MAX_REGISTERS` is only used in `writeprotect.c` AFAICS, so it would belong […]
I've made it the last element, that should be a bit more robust. It's useful if you want to iterate over all registers that flashrom knows about, though I don't have another example of doing that outside writeprotect right now.
File spi25_statusreg.c:
https://review.coreboot.org/c/flashrom/+/58475/comment/3f1900ef_cc294e44
PS14, Line 51: int result = spi_send_multicommand(flash, cmds);
> What do you mean? What could cause problems?
I think you're referring to the fact that it declares a variable in the middle of the function? There's quite a lot of other code like that, it shouldn't cause any new problems.
https://review.coreboot.org/c/flashrom/+/58475/comment/ffe275e5_24adef03
PS14, Line 126:
> should be a tab
Done
https://review.coreboot.org/c/flashrom/+/58475/comment/15fbcf91_ecfe6b6c
PS14, Line 134:
> nit: double blank line
Done
https://review.coreboot.org/c/flashrom/+/58475/comment/8770b6c6_08bea653
PS14, Line 149:
> nit: double blank line
Done
https://review.coreboot.org/c/flashrom/+/58475/comment/4585c851_7818ee9f
PS14, Line 194: spi_write_register failed.
> Would be good to indicate which register failed: […]
Done
https://review.coreboot.org/c/flashrom/+/58475/comment/1a0216b7_f39b55f1
PS14, Line 207: spi_write_register failed.
> Ditto
Done
File spi25_statusreg.c:
https://review.coreboot.org/c/flashrom/+/58475/comment/4a646003_8ff8b64e
PS15, Line 63: */
> I have doubts that this is true for any modern chip. "allow running RDSR […]
I'm inclined to agree, this code dates back to 2010 in commit 174f55bdec62, which doesn't mention any specific chip, but says that some chips take >100ms, which implies that the initial delay wasn't doing anything useful even then.
We can probably just switch everything over to repeated polling, WDYT?
https://review.coreboot.org/c/flashrom/+/58475/comment/7d58e45b_0890c29c
PS15, Line 84: *
> Please drop this dangling asterisk or add a line break after the first […]
Done
https://review.coreboot.org/c/flashrom/+/58475/comment/3d71c81b_5975355c
PS15, Line 89: }
> Why not use a switch/case? It would make it easier to provide proper error […]
The next commit adds nested ifs for handling chips with different ways of writing the same register, and they cant be converted to a switch/case very cleanly.
https://review.coreboot.org/c/flashrom/+/58475/comment/f17825b5_821c2d14
PS15, Line 91: not supported by chip\
> Or not supported by this function?
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/58475
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0a3951bbf993f2d8d830143b29d3ce16cc6901d7
Gerrit-Change-Number: 58475
Gerrit-PatchSet: 16
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: 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: Fri, 24 Dec 2021 06:47:15 +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>
Gerrit-MessageType: comment