April 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: pony_spi.c: Extract out get_params to simplify init
by Angel Pons (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Light, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63130 ) Change subject: pony_spi.c: Extract out get_params to simplify init ...................................................................... Patch Set 3: (7 comments) File pony_spi.c: https://review.coreboot.org/c/flashrom/+/63130/comment/8833d157_f0edb292 PS3, Line 153: { nit: add space before opening brace https://review.coreboot.org/c/flashrom/+/63130/comment/10aeaede_615bf794 PS3, Line 167: bool have_device; > From my point of view: Review doesn't scale well with the size of > a patch. Even if it's a very simple patch hunk, every additional > hunk can distract from the error-prone part of a patch. Doing fixes > and simple changes like this in advance works well, FWIW. +1 For me, it's very easy to miss something when reviewing commits that do multiple things at once. I'd rather review multiple commits where each does one and only one thing. Whenever I want/need to make significant code changes, I draft in advance a "plan of action". I use several criteria to segment the changes into commits, one of them is "at most one thing/reason/idea per commit". In this case, "simplify `pony_spi` init logic" and "use `bool` type for boolean variables" are two different things, and they are conceptually independent: disregarding merge conflicts, it's possible to do them in either order, or only do one of them. When accounting for merge conflicts, I start with the easiest (most likely to be approved) commits, so that I don't have to manually rebase them if another commit needs to be fixed. That being said, there's no need to over-split things. For example, I'd retype both `have_device` and `have_prog` in the same commit, because the rationale is the same. TL;DR: Please retype the variables in a separate commit (preferably before the "extract `get_params`" commit to streamline the submission process). https://review.coreboot.org/c/flashrom/+/63130/comment/523207c5_d0f3c3b6 PS3, Line 171: return 1; If the `dev` parameter is correct (i.e. `have_device` is true) but `get_params()` returns -1 because of an error with the `type` parameter, the serial port (what `serialport_shutdown()` releases) will leak. https://review.coreboot.org/c/flashrom/+/63130/comment/3b064e8c_49fe92dc PS3, Line 181: return 1; Serial port can also leak here. https://review.coreboot.org/c/flashrom/+/63130/comment/229d3eda_23804449 PS3, Line 190: serialport_shutdown > I think you are mis-interpreting the diff presented by gerrit Anastasia. This line is OK here, but it's missing in some other return paths. https://review.coreboot.org/c/flashrom/+/63130/comment/54be3b14_cde22584 PS3, Line 232: true This is unrelated. Please do it in a separate commit. https://review.coreboot.org/c/flashrom/+/63130/comment/9797c619_2ea34563 PS3, Line 237: true Ditto -- To view, visit https://review.coreboot.org/c/flashrom/+/63130 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I364febc05c870683cbad114583762b0c006f4bac Gerrit-Change-Number: 63130 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> 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: Light <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 11:26: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

1 0

Change in flashrom[master]: pony_spi.c: Extract out get_params to simplify init
by Paul Menzel (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Edward O'Callaghan, Light, Anastasia Klimchuk. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63130 ) Change subject: pony_spi.c: Extract out get_params to simplify init ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/63130/comment/77310740_a9f5f7d3 PS3, Line 9: `commit caa0335114a81` Nit: No markup needed. Maybe even add the commit message summary in () after it. -- To view, visit https://review.coreboot.org/c/flashrom/+/63130 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I364febc05c870683cbad114583762b0c006f4bac Gerrit-Change-Number: 63130 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Apr 2022 05:43:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Add Jasper Lake support
by Edward O'Callaghan (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Nico Huber, Subrata Banik, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62282 ) Change subject: ichspi: Add Jasper Lake support ...................................................................... Patch Set 10: (2 comments) Patchset: PS4: > > Unresolving then because it does look like a pch300 although I am not 100% on every tiny idiosyncr […] You are right `NM` is confusing, can we comment it better? Possibly copy what you just said there near verbatim into the `ich_number_of_masters()` function? OK, I picked to follow pch300 in `prettyprint_ich_descriptor_master()` and fixed that up. `ich_number_of_masters()` was left untouched. I wonder how much of this declarative information about the differences could be encoded into just a big table instead of scattered over heaps of switch statements in the control flow itself? A lot of it doesn't seem dynamic just XXX has 3 and YYY has 4 of "thing". Resolved? File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62282/comment/62044ba6_c49c4d9d PS8, Line 1061: CHIPSET_ELKHART_LAKE > Google should have access too. Since ~SKL, both the SPI guide and FIT are […] Long story about the access ACL's there.. Can talk out of band about it || summarised as 'a lot of nonsense'. -- To view, visit https://review.coreboot.org/c/flashrom/+/62282 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib942d0b8942fe0a991b2af0b187414818485153d Gerrit-Change-Number: 62282 Gerrit-PatchSet: 10 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 03 Apr 2022 00:30:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Add Jasper Lake support
by Edward O'Callaghan (Code Review) 03 Apr '22

03 Apr '22

Attention is currently required from: Subrata Banik, Edward O'Callaghan, Angel Pons. Hello Sam McNally, build bot (Jenkins), Subrata Banik, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62282 to look at the new patch set (#10). Change subject: ichspi: Add Jasper Lake support ...................................................................... ichspi: Add Jasper Lake support Additionally, utilize CSSO (CPU Soft Strap Offset) to uniquely detect the chipset when the CSSL (CPU Soft Strap Length) field default value (0x03) on Jasper Lake is the same as Elkhart Lake. BUG=b:221175960 TEST=dedede with `flashrom -p internal --flash-size`. ``` $ flashrom -VVV -p internal --ifd -i fd -i bios -r /tmp/filename.rom <snip> Enabling hardware sequencing by default for 100+ series PCH. OK. No board enable found matching coreboot IDs vendor="Google", model="Magolor". The following protocols are supported: Programmer-specific. Probing for Programmer Opaque flash chip, 0 kB: Chip identified: GD25Q127C/GD25Q128C Hardware sequencing reports 1 attached SPI flash chip with a density of 16384 kB. There is only one partition containing the whole address space (0x000000 - 0xffffff). There are 4096 erase blocks with 4096 B each. Added layout entry 00000000 - 00ffffff named complete flash Found GigaDevice flash chip "GD25Q127C/GD25Q128C" (16384 kB, Programmer-specific) mapped at physical address 0x0000000000000000. Found GigaDevice flash chip "GD25Q127C/GD25Q128C" (16384 kB, Programmer-specific). This chip may contain one-time programmable memory. flashrom cannot read and may never be able to write it, hence it may not be able to completely clone the contents of this chip (see man page for details). Reading Status register Block protection is disabled. Reading ich descriptor... Reading 4096 bytes starting at 0x000000. done. Assuming chipset 'Jasper Lake'. Added layout entry 00000000 - 00000fff named fd Added layout entry 00381000 - 00ffffff named bios Added layout entry 00001000 - 00380fff named me restore_power_management: Re-enabling power management. Using regions: "bios", "fd". Reading Status register Block protection is disabled. Reading flash... 0x381000-0xffffff:R Reading 13103104 bytes starting at 0x381000. 000000-0x0fff:R Reading 4096 bytes starting at 0x000000. done. restore_power_management: Re-enabling power management. SUCCESS Restoring PCI config space for 00:1f:5 reg 0xdc restore_power_management: Re-enabling power management. ``` Change-Id: Ib942d0b8942fe0a991b2af0b187414818485153d Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M chipset_enable.c M ich_descriptors.c M ichspi.c M programmer.h M util/ich_descriptors_tool/ich_descriptors_tool.c 5 files changed, 36 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/62282/10 -- To view, visit https://review.coreboot.org/c/flashrom/+/62282 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib942d0b8942fe0a991b2af0b187414818485153d Gerrit-Change-Number: 62282 Gerrit-PatchSet: 10 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: pony_spi.c: Extract out get_params to simplify init
by Nico Huber (Code Review) 02 Apr '22

02 Apr '22

Attention is currently required from: Edward O'Callaghan, Light, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63130 ) Change subject: pony_spi.c: Extract out get_params to simplify init ...................................................................... Patch Set 3: (1 comment) File pony_spi.c: https://review.coreboot.org/c/flashrom/+/63130/comment/ac5dcf12_bccea0c8 PS3, Line 167: bool have_device; > A bit of nit isn't it? Is there a solid technical reason here or just personal preference? From my point of view: Review doesn't scale well with the size of a patch. Even if it's a very simple patch hunk, every additional hunk can distract from the error-prone part of a patch. Doing fixes and simple changes like this in advance works well, FWIW. -- To view, visit https://review.coreboot.org/c/flashrom/+/63130 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I364febc05c870683cbad114583762b0c006f4bac Gerrit-Change-Number: 63130 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Light <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Light <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 02 Apr 2022 14:19:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Implement file-based locking semantics
by Daniel Campello (Code Review) 02 Apr '22

02 Apr '22

Attention is currently required from: Hung-Te Lin, Nico Huber, Subrata Banik, Edward O'Callaghan. Daniel Campello has uploaded a new patch set (#17) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/62213 ) Change subject: flashrom.c: Implement file-based locking semantics ...................................................................... flashrom.c: Implement file-based locking semantics This upstreams the ChromiumOS implementation of file-based locking for multiple instances of flashrom that could be spawned either from libflashrom (perhaps by fwupd for example) and user cli as another example. Since flashrom is programming singleton state of hardware from userspace there is no way to exclusively own the address space and therefore a file-based locking semantic is considered here. BUG=b:217629892,b:215255210 BRANCH=none TEST=nm -gD /build/brya/usr/lib64/libflashrom.so | grep "flock" Test futility update with multiple instances of flashrom running. Change-Id: I19cb4e3bf14caeb67c3e8100a20395b264c5113a Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M Makefile A big_lock.c A big_lock.h A file_lock.c M flashrom.c A ipc_lock.h M meson.build M tests/chip.c M tests/include/test.h M tests/lifecycle.c M tests/meson.build M tests/tests.c 12 files changed, 492 insertions(+), 17 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/62213/17 -- To view, visit https://review.coreboot.org/c/flashrom/+/62213 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I19cb4e3bf14caeb67c3e8100a20395b264c5113a Gerrit-Change-Number: 62213 Gerrit-PatchSet: 17 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> 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-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: tests/: wrap flock() and ftruncate()
by Daniel Campello (Code Review) 02 Apr '22

02 Apr '22

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62312 ) Change subject: tests/: wrap flock() and ftruncate() ...................................................................... Patch Set 11: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/62312/comment/71042151_ca94a9bc PS6, Line 7: wrap flock() and ftruncate() > I think this should be its own commit and merged regardless. […] I am not sure about this, why stop on only I/O ops and not write mocks for every syscall? -- To view, visit https://review.coreboot.org/c/flashrom/+/62312 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I57e7843a4d94c0a7200690d4c1d9e4194b9f83c5 Gerrit-Change-Number: 62312 Gerrit-PatchSet: 11 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 02 Apr 2022 13:55:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: use MOCK_FD instead of NON_ZERO
by Daniel Campello (Code Review) 02 Apr '22

02 Apr '22

Attention is currently required from: Anastasia Klimchuk. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63193 ) Change subject: tests: use MOCK_FD instead of NON_ZERO ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/63193/comment/7b884d31_ebb7c75d PS3, Line 9: return a non-negative value for : the file descriptor expected from open operations. > Actually, was this intentional? […] This could not be intentional. It would mean that open() would always fail unless a mock is defined for the test. At least we know that the linux_spi is using the default open wrapper with /dev/null as path (if the code checked for (fd < 0) this would have caused the test to fail) -- To view, visit https://review.coreboot.org/c/flashrom/+/63193 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib6bac051852aea2465665a6fd669b7f5e3772985 Gerrit-Change-Number: 63193 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 02 Apr 2022 13:53:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: assert pathname and flags when calling open()
by Daniel Campello (Code Review) 02 Apr '22

02 Apr '22

Attention is currently required from: Sam McNally. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63227 ) Change subject: tests: assert pathname and flags when calling open() ...................................................................... Patch Set 6: (2 comments) File tests/tests.c: https://review.coreboot.org/c/flashrom/+/63227/comment/a404b6ba_abd3579a PS6, Line 77: wrap_open_and_friends > this may have unexpected behavior when `(get_io()->open != NULL)&&(get_io()->open_state != NULL)`. open_state in not used when open != NULL (this function returns from within this if case) https://review.coreboot.org/c/flashrom/+/63227/comment/19990c0d_b3d6b86e PS6, Line 81: ) > `&& !get_io()->open)` see above -- To view, visit https://review.coreboot.org/c/flashrom/+/63227 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb Gerrit-Change-Number: 63227 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Comment-Date: Sat, 02 Apr 2022 13:51:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Add Jasper Lake support
by Nico Huber (Code Review) 02 Apr '22

02 Apr '22

Attention is currently required from: Subrata Banik, Edward O'Callaghan, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62282 ) Change subject: ichspi: Add Jasper Lake support ...................................................................... Patch Set 9: (2 comments) Patchset: PS4: > Unresolving then because it does look like a pch300 although I am not 100% on every tiny idiosyncrasy. > > If `ich_number_of_masters()` needs adjusting then so does the switch in `prettyprint_ich_descriptor_master()` too. NM has always been vaguely defined. The original assumption was that it's 0 based. This didn't work for some niche platforms, IIRC starting with Lewisburg. AIUI, the value was never interpreted by the hardware and only for FIT's convenience. It seems possible that not even FIT cares about it anymore and we are chasing ghosts (i.e. we might have adapted the code to make it work while we actually read a garbage value). One possible way out of the dilemma would be to let ich_number_of_masters() return static values per platform (or just make it a constant array). Simply use the maximum a descriptor generation supports. From the SPI guides it looks like the arrays like FLMSTR are not dynamic, i.e. always have the same length for a given generation. We should have a look at some descrip- tors to confirm that. Probably the same for NR on older platforms. > > Thing is, that has: > ``` > const char *const master_names[] = { > "BIOS", "ME", "GbE", "unknown", "EC", > }; > ``` > but the datasheet says on page 42 - [BIOS, ME, GbE, EC] and states there are 4 masters. Nothing indicates if to interpret "number of masters" from the flash descriptor as so called- 0-based or 1-based. Therefore I just assumed `cont->NM`. If in doubt, it's often good to assume that nothing changed. What makes things harder is that we can't derive how optional things are considered like that "EC" master. I don't think I've ever worked with a board that makes use of it. > > Open to some guidance here? My concern is that I got those two functions wrong. Sometimes it's easiest to do what the existing code for the closest relative does. If it needs to be fixed later, it at least avoids arguments like "why did JSL differ, let's better not touch the code". File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62282/comment/428fe818_20fc729b PS8, Line 1061: CHIPSET_ELKHART_LAKE > Thanks Nico that is super useful ! I didn't have access to EHL I was asking if Subrata could ask a c […] Google should have access too. Since ~SKL, both the SPI guide and FIT are part of the CSE/(CS)ME/TXE firmware packages. It's not always easy to find on intel.com, took me hours to find one for JSL. For newer platforms the following search strings might give results "... Lake CSE" "... Lake CSME" "... Lake BKC". It can also help to know the version for a given platform, for this it's best to query the experts: https://www.win-raid.com/t596f39-Intel-Converged-Security-Management-Engine… (still not easy to keep track of codenames, who would have thought that the EHL PCH is Mule Creek Canyon?) -- To view, visit https://review.coreboot.org/c/flashrom/+/62282 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib942d0b8942fe0a991b2af0b187414818485153d Gerrit-Change-Number: 62282 Gerrit-PatchSet: 9 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sat, 02 Apr 2022 13:45:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2022