Attention is currently required from: Nico Huber, Paul Menzel, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59237 )
Change subject: tests: Set up mock chip memory in consistent and predictable way
......................................................................
Patch Set 6: Code-Review+1
(2 comments)
File tests/chip.c:
https://review.coreboot.org/c/flashrom/+/59237/comment/29291648_c2e54296
PS6, Line 33: .buf = { MOCK_CHIP_CONTENT },
This only initialises the first element of `buf`, the rest of the array is initialised to 0. I see it gets filled later on, but I find this potentially misleading.
I'd remove the static initialiser (or leave it to explicitly initialise to 0) and put the actual initialisation code in a helper function just below.
https://review.coreboot.org/c/flashrom/+/59237/comment/0541c335_3cc7c8fe
PS6, Line 97: memset(&g_chip_state.buf[0], MOCK_CHIP_CONTENT, MOCK_CHIP_SIZE);
How about:
memset(g_chip_state.buf, MOCK_CHIP_CONTENT, sizeof(g_chip_state.buf));
--
To view, visit https://review.coreboot.org/c/flashrom/+/59237
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0d7623a601c207bfc62d54ab89d94cda56d85871
Gerrit-Change-Number: 59237
Gerrit-PatchSet: 6
Gerrit-Owner: 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: 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: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 24 Jan 2022 13:40:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61286 )
Change subject: flashrom.c: Move {read,write}_buf_from_include_args() to layout
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> These two functions are best situated within cli_common once read_flash_to_file() has been eliminate […]
Actually, they are dead code ATM, so I find it hard to reason about intermediate
steps.
If I'd write this (per-region file arguments) from scratch, I'd probably go
either one of these two ways:
1. Keep everything file related in CLI code. CLI would have to maintain its own
mapping from region names to file names (to implement the two functions with the
libflashrom API, we'd need a public API to walk the layout entries).
2. Add at set of optional, file-aware libflashrom APIs, e.g. libflashrom_file.[hc].
Something that let's the API user add file names to layout entries and then the
two functions that read/write everything from/to files.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61286
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I61d4460ea26e07b67c99baeae5cb3840c25cb913
Gerrit-Change-Number: 61286
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: 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-Comment-Date: Mon, 24 Jan 2022 13:04:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Thomas Heijligen, Paul Menzel, Edward O'Callaghan, Peter Marheine.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61194 )
Change subject: hwaccess: fix build on non-x86 targets
......................................................................
Patch Set 7: Code-Review+1
(1 comment)
File hwaccess_x86_io.c:
https://review.coreboot.org/c/flashrom/+/61194/comment/a62e0fb1_4db42191
PS7, Line 82: /* PCI port I/O support is unimplemented on PPC/MIPS and unavailable on ARM. */
> Where did this go?
Ah:
The comment on the stub implementation of rget_io_perms() is also
modified to remove references to non-x86 platforms, since that file is
only built on x86 now.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e
Gerrit-Change-Number: 61194
Gerrit-PatchSet: 7
Gerrit-Owner: Peter Marheine <pmarheine(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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 24 Jan 2022 12:43:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Thomas Heijligen, Paul Menzel, Edward O'Callaghan, Peter Marheine.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61194 )
Change subject: hwaccess: fix build on non-x86 targets
......................................................................
Patch Set 7:
(1 comment)
File hwaccess_x86_io.c:
https://review.coreboot.org/c/flashrom/+/61194/comment/8b25fe85_756c6d6d
PS7, Line 82: /* PCI port I/O support is unimplemented on PPC/MIPS and unavailable on ARM. */
Where did this go?
--
To view, visit https://review.coreboot.org/c/flashrom/+/61194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e
Gerrit-Change-Number: 61194
Gerrit-PatchSet: 7
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 24 Jan 2022 12:38:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60711 )
Change subject: Add Elkhart Lake support
......................................................................
Patch Set 2:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/60711/comment/961cf5ae_2cd98383
PS2, Line 15: TEST=Read and flash complete flash on Siemens MC EHL1
> Some tests with --ifd and writing only specific regions would be nice […]
I have tried writing just the BIOS region with
flashrom -p internal --ifd -i bios -w coreboot.rom
and it went all smooth so far.
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/60711/comment/15832eed_b802c7ef
PS1, Line 1033: return CHIPSET_500_SERIES_TIGER_POINT;
> Please also test if it detects the right platform. Should be easy […]
You nailed it, Nico.
I have tried 'util/ich_descriptors_tool/ich_descriptors_tool -f coreboot.rom' with my latest mc_ehl1 coreboot image and got:
Unknown flash descriptor, assuming 500 series compatibility.
Assuming chipset '500 series Tiger Point'.
A closer look showed that neither FLMAP1 nor FLMAP2 matches the documented settings in the SPI guide (especially the "Set to xx" comment in the register description). This is true for at least two different FIT versions I have tested as well as for the UEFI release for the Elkhart Lake CRB.
I will reach out to Intel to clarify the discrepancy so that we have a chance to make it right. Will report any update here.
--
To view, visit https://review.coreboot.org/c/flashrom/+/60711
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I711e39a3ec9cd7098389231eaa1cb864d615a475
Gerrit-Change-Number: 60711
Gerrit-PatchSet: 2
Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.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-Comment-Date: Mon, 24 Jan 2022 07:14:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-MessageType: comment