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
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
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
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
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/+/58475 )
Change subject: spi25_statusreg: make register read/write functions generic
......................................................................
Patch Set 19:
(4 comments)
File spi25_statusreg.c:
https://review.coreboot.org/c/flashrom/+/58475/comment/8e41c1f8_d7b6e6da
PS14, Line 51: int result = spi_send_multicommand(flash, cmds);
> I think you're referring to the fact that it declares a variable in the middle of the function? Ther […]
Ack
File spi25_statusreg.c:
https://review.coreboot.org/c/flashrom/+/58475/comment/00d231f3_77c17f09
PS15, Line 63: */
> Changed in latest patch set.
Well, it's one thing to avoid a kludge for new use cases but something
different to change behavior for existing use cases. Unless we want
to test a lot of 15+ years old chips, please keep the behavior for SR1.
I'd be ok to risk to break things, but that would have to be a commit
on its own, e.g. after this series, a patch that removes the kludge even
for SR1.
https://review.coreboot.org/c/flashrom/+/58475/comment/c06493a7_69fdab9d
PS15, Line 84: *
> Done
Doesn't look done.
I'm not sure but I guess checkpatch would see this. Please just try.
See [1] for Linus' reasoning and [2] for the style choices in coreboot
(which avoids C++ comments but should be compatible with the style used
here).
[1] https://lkml.org/lkml/2016/7/8/625
[2] https://doc.coreboot.org/contributing/coding_style.html#commentinghttps://review.coreboot.org/c/flashrom/+/58475/comment/74beeeae_e612a757
PS15, Line 89: }
> The next commit adds nested ifs for handling chips with different ways of writing the same register, […]
Are you saying one can't use an if inside switch/case? I've looked
ahead and I don't see any real problem.
--
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: 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: 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 14:51:54 +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
Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Peter Marheine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59275 )
Change subject: pcidev: Avoid internal programmer relying on pacc global
......................................................................
Patch Set 4:
(1 comment)
File pcidev.c:
https://review.coreboot.org/c/flashrom/+/59275/comment/72d3158c_a727f9cc
PS4, Line 154: start
Doesn't it stall on the second invocation? i.e. should this be `start->next`?
--
To view, visit https://review.coreboot.org/c/flashrom/+/59275
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id83bfd41f785f907e52a65a6689e8c7016fc1b77
Gerrit-Change-Number: 59275
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Fri, 14 Jan 2022 23:13:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/60087 )
Change subject: dediprog: wait for spi bulk read xfers to finish
......................................................................
dediprog: wait for spi bulk read xfers to finish
dediprog_bulk_read_poll()'s finish argument allows it to be used in two
distinct cases: where dediprog_bulk_read_poll will be called as part of
a loop (finish=0) and where dediprog_bulk_read_poll should wait for all
outstanding transfers to finish (finish=1). In both cases,
dediprog_bulk_read_poll() calls libusb to process events with a 10
second timeout.
After dediprog_spi_bulk_read() has queued the last transfers, it calls
dediprog_bulk_read_poll() with finish=0 when it should be finish=1.
finish=0 just happens to work because frequently the transfers finish in
the 10 second timeout.
Signed-off-by: Rick Altherr <rick(a)oxidecomputer.com>
Change-Id: If7cb541742c8620358c8e04275d8316131b2d1ab
Reviewed-on: https://review.coreboot.org/c/flashrom/+/60087
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
---
M dediprog.c
1 file changed, 1 insertion(+), 1 deletion(-)
Approvals:
build bot (Jenkins): Verified
Nico Huber: Looks good to me, approved
Angel Pons: Looks good to me, approved
diff --git a/dediprog.c b/dediprog.c
index 939673c..8a05a7c 100644
--- a/dediprog.c
+++ b/dediprog.c
@@ -526,7 +526,7 @@
goto err_free;
}
/* Wait for transfers to finish. */
- if (dediprog_bulk_read_poll(dp_data->usb_ctx, &status, 0))
+ if (dediprog_bulk_read_poll(dp_data->usb_ctx, &status, 1))
goto err_free;
/* Check if everything has been transmitted. */
if ((status.finished_idx < count) || status.error)
--
To view, visit https://review.coreboot.org/c/flashrom/+/60087
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If7cb541742c8620358c8e04275d8316131b2d1ab
Gerrit-Change-Number: 60087
Gerrit-PatchSet: 2
Gerrit-Owner: Rick Altherr <kc8apf(a)kc8apf.net>
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-MessageType: merged
Attention is currently required from: Rick Altherr.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60087 )
Change subject: dediprog: wait for spi bulk read xfers to finish
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Patchset:
PS1:
> Looks sensible to me, but I don't have a dediprog to test.
It's actually a recent regression. Sneaked into a revision of CB:56414.
--
To view, visit https://review.coreboot.org/c/flashrom/+/60087
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If7cb541742c8620358c8e04275d8316131b2d1ab
Gerrit-Change-Number: 60087
Gerrit-PatchSet: 1
Gerrit-Owner: Rick Altherr <kc8apf(a)kc8apf.net>
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-Attention: Rick Altherr <kc8apf(a)kc8apf.net>
Gerrit-Comment-Date: Fri, 14 Jan 2022 15:41:12 +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: Werner Zeh.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60711 )
Change subject: Add Elkhart Lake support
......................................................................
Patch Set 1: Code-Review+1
(10 comments)
Patchset:
PS1:
Nice! looks almost ready :)
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/60711/comment/d7633cdc_5e8ad909
PS1, Line 709: boot_straps = boot_straps_pch500;
The EDS says `bbs == 1` would be "UFS". Not sure how that's connected.
Is it related to eSPI?
https://review.coreboot.org/c/flashrom/+/60711/comment/8aa841ba_97c9aebe
PS1, Line 995: dev
So AFAICS, this is already 1f.5. We could save us the dance to find it
(the PCH100 code assumes it is hidden), e.g. extract the code in
enable_flash_pch100_or_c620() that works on `spi_dev` into a function
that we call here directly.
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/60711/comment/acd5afa7_31fcb7bc
PS1, Line 538: IFWI
Intel likes to haunt us with this it seems. Now all the non-BIOS firmware is
back in the TXE/ME region, but they still call it IFWI. Maybe we should just
change this back to BIOS. After all, everybody is calling it the BIOS region.
https://review.coreboot.org/c/flashrom/+/60711/comment/07f104ed_507f4713
PS1, Line 1033: return CHIPSET_500_SERIES_TIGER_POINT;
If the SPI guide is correct and I followed it, we would end up on this path.
We can identify EHL by CSSO == 0x6c (please check that in your descriptor), i.e.
if (content->CSSL == 0x11) {
if (content->CSSO == 0x68)
return CHIPSET_500_SERIES_TIGER_POINT;
if (content->CSSO == 0x6c)
return CHIPSET_ELKHART_LAKE;
}
https://review.coreboot.org/c/flashrom/+/60711/comment/d8037f2d_edc231ea
PS1, Line 1055: case CHIPSET_GEMINI_LAKE:
EHL here?
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/60711/comment/1c70e168_d2c3a495
PS1, Line 452: pprint_reg(HSFS, FLOCKDN, reg_val, "\n");
Looks like there are several new SAF (slave-attached flash?) bits. We
must have missed that when adding APL already. Or actually, missed
the whole function :-(
I don't mind leaving them out as long as the printed bits are correct.
https://review.coreboot.org/c/flashrom/+/60711/comment/bb51cd48_b6871c2e
PS1, Line 464: case CHIPSET_500_SERIES_TIGER_POINT:
Looks like we missed APL/GLK here? EHL should go here too it seems.
https://review.coreboot.org/c/flashrom/+/60711/comment/1b8bf0b8_05ee0633
PS1, Line 1774: case CHIPSET_ELKHART_LAKE:
This is tricky, the additional regs are at 0xe0. So it actually has 16.
https://review.coreboot.org/c/flashrom/+/60711/comment/ccdc3fba_8b023875
PS1, Line 2027: ich_gen == CHIPSET_ELKHART_LAKE)) {
Hmmm, would fit better below. Or maybe just merge the two if's. The idea
was to have an accurate message, but becomes harder with every new chip.
--
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: 1
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-Attention: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Fri, 14 Jan 2022 14:27:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Jonathan Zhang.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54965 )
Change subject: Add support for Intel Emmitsburg PCH
......................................................................
Patch Set 3:
(1 comment)
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/54965/comment/34d05a85_5f5e98f8
PS3, Line 2076: enable_flash_c620
> Looking at the EDS (606161, rev 1.3), it seems closer to 300 series.
> At least wrt. the FREG registers.
Will anybody look into it?
--
To view, visit https://review.coreboot.org/c/flashrom/+/54965
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2a1bb7467e693d1583aa885fa0e277075edd4a3e
Gerrit-Change-Number: 54965
Gerrit-PatchSet: 3
Gerrit-Owner: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: David Hendricks
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.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: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Comment-Date: Fri, 14 Jan 2022 12:20:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Jonathan Zhang <jonzhang(a)fb.com>
Comment-In-Reply-To: David Hendricks
Gerrit-MessageType: comment