Attention is currently required from: Felix Singer, Angel Pons.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71269 )
Change subject: programmer.h: Guard against sending spi commands on non-spi mst
......................................................................
Patch Set 2:
(1 comment)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/71269/comment/79edb5d5_14e8c830
PS1, Line 1937: spi_master_no_4ba_modes
> Wouldn't it make more sense to add the bustype check inside this function?
sort of but not really. This is a chip bustype check not a master bustype check.
Adding a predicate to `spi_master_no_4ba_modes()` is not related to preventing the subsequent calls to `spi_{enter,exit}_4ba()`.
I think what you are asking for is a `spi_chip_4ba()` func which I have now done?
--
To view, visit https://review.coreboot.org/c/flashrom/+/71269
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7cce4f9c032d33c01bf616e27a50b9727a40fe1b
Gerrit-Change-Number: 71269
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 04 Jan 2023 01:28:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Edward O'Callaghan.
Hello Felix Singer, build bot (Jenkins), Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/71269
to look at the new patch set (#2).
Change subject: programmer.h: Guard against sending spi commands on non-spi mst
......................................................................
programmer.h: Guard against sending spi commands on non-spi mst
Validate (flash->chip->bustype == BUS_SPI) as ich copies the
chip flags in the opaque master and tries incorrectly
to issue 4BA commands which results in failure.
The issue was detected only in the case of chips >16MB, in
this case 'W25Q256FV' that has the feature bits:
```
.feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA_ENTER_WREN |
FEATURE_4BA_EAR_C5C8 | FEATURE_4BA_READ | FEATURE_4BA_FAST_READ |
FEATURE_WRSR2,
```
The regression was noticed from,
commit 0741727925b841c2479b993204ce58c5eb75185a ichspi.c: Read chip ID and use it to populate `flash->chip`
TEST=In the case of 'W25Q256FV' on TigerLake.
Change-Id: I7cce4f9c032d33c01bf616e27a50b9727a40fe1b
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M flashrom.c
M include/programmer.h
2 files changed, 33 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/69/71269/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/71269
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7cce4f9c032d33c01bf616e27a50b9727a40fe1b
Gerrit-Change-Number: 71269
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Thomas Heijligen, Arthur Heymans.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71565 )
Change subject: sb600spi.c: Move promontory code into a mmap_read=yes mode
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/71565/comment/70f0f78d_8675d2de
PS2, Line 10: This is not
: hardware specific
I believe it is hardware specific to controllers after a certain generation. i.e., supported on SPI100 controllers (Zen etc) however not supported on pre-SPI100 controllers was my understanding.
--
To view, visit https://review.coreboot.org/c/flashrom/+/71565
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I13eb5a646d3569170b3911ae7b3127cd3e6022aa
Gerrit-Change-Number: 71565
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Tue, 03 Jan 2023 23:51:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Sam McNally, Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71576 )
Change subject: chipset_enable.c: Add TL UP3 and UP4/Y id's
......................................................................
Patch Set 1:
(1 comment)
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/71576/comment/acd6174a_b5a5ac11
PS1, Line 2083: {0x8086, 0xa083, B_S, DEP, "Intel", "Tiger Lake U Premium 3", enable_flash_pch500},
> As far as I can tell, 0xa083 isn't used after all, so we don't need to add this.
Let's keep it, but mark it as untested (change `DEP` to `NT`).
--
To view, visit https://review.coreboot.org/c/flashrom/+/71576
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I77120612f7a770ae1319f4cd82913fa465f355f5
Gerrit-Change-Number: 71576
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Tue, 03 Jan 2023 06:23:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sam McNally <sammc(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71576 )
Change subject: chipset_enable.c: Add TL UP3 and UP4/Y id's
......................................................................
Patch Set 1:
(2 comments)
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/71576/comment/339e3db0_c567540c
PS1, Line 2083: {0x8086, 0xa083, B_S, DEP, "Intel", "Tiger Lake U Premium 3", enable_flash_pch500},
As far as I can tell, 0xa083 isn't used after all, so we don't need to add this.
https://review.coreboot.org/c/flashrom/+/71576/comment/d1f3ec48_db6bf680
PS1, Line 2084: U Premium 4/Y
If we follow the final naming scheme this should be "Tiger Lake Premium UP4". If we follow earlier platforms, and the coreboot scheme, then "Tiger Lake Y Premium".
--
To view, visit https://review.coreboot.org/c/flashrom/+/71576
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I77120612f7a770ae1319f4cd82913fa465f355f5
Gerrit-Change-Number: 71576
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Tue, 03 Jan 2023 03:50:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71624 )
Change subject: internal.c: Do cb_parse_table() in preinit
......................................................................
Patch Set 1:
(1 comment)
File include/programmer.h:
https://review.coreboot.org/c/flashrom/+/71624/comment/2a1d035a_2b63228f
PS1, Line 35: struct {
: const char *cb_vendor;
: const char *cb_model;
: } internal;
This should be a programmer-defined opaque struct, pointed to by `struct programmer_cfg`.
--
To view, visit https://review.coreboot.org/c/flashrom/+/71624
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7f4fbb4a8bc362c4ac27a5ab2472551a5467e240
Gerrit-Change-Number: 71624
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Jan 2023 13:11:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71623 )
Change subject: board_enables: Couple prog cfg with board cfg
......................................................................
Patch Set 1:
(2 comments)
Patchset:
PS1:
It's hard to say if all the plumbing is needed, but no idea how to make it better.
File internal.c:
https://review.coreboot.org/c/flashrom/+/71623/comment/e1b8040f_e61d8481
PS1, Line 270: board_handle_before_superio((struct programmer_cfg *)cfg);
Ugh, it'd be better to avoid casting away const... Quick solution would be to make a mutable local copy:
struct programmer_cfg mut_cfg = *cfg;
Then use `&mut_cfg` as argument instead of `(struct programmer_cfg *)cfg`.
--
To view, visit https://review.coreboot.org/c/flashrom/+/71623
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7058a693e714a6966a842ae97cc8da7296e63e5e
Gerrit-Change-Number: 71623
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Jan 2023 13:08:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71622 )
Change subject: dmi.c: Pass is_laptop by ref into dmi
......................................................................
Patch Set 1:
(2 comments)
File dmi.c:
https://review.coreboot.org/c/flashrom/+/71622/comment/2bfda70c_1c51374e
PS1, Line 158: *is_laptop = 2;
For another patch: This should be an enum.
https://review.coreboot.org/c/flashrom/+/71622/comment/9efed287_947fd226
PS1, Line 402: is_laptop
nit: update comment too?
--
To view, visit https://review.coreboot.org/c/flashrom/+/71622
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3d5ad6c0623269492d775a99a947fd6fe26c5f91
Gerrit-Change-Number: 71622
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Jan 2023 13:04:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71621 )
Change subject: internal.c: Have preinit hook do cpu and bus init
......................................................................
Patch Set 1: Code-Review+1
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/71621/comment/bcbd22eb_8670ea2e
PS1, Line 7: cpu
Not sure why it says "Processor", but it seems inaccurate. Not this change's problem.
https://review.coreboot.org/c/flashrom/+/71621/comment/7cedb353_513ed060
PS1, Line 10: initalisation
Insert an `i` after the first 4 letters: init_i_alisation
https://review.coreboot.org/c/flashrom/+/71621/comment/1c54aa0a_2dae6194
PS1, Line 11: initalise
Insert an `i` after the first 4 letters: init_i_alise
File internal.c:
https://review.coreboot.org/c/flashrom/+/71621/comment/301c4c48_bf08dd51
PS1, Line 245: if (try_mtd(cfg) == 0) {
: ret = 0;
: goto internal_init_exit;
: }
We don't need to initialise PCI access or perform flash enables if we're using MTD, do we?
Actually, we want to run the flash enables for MTD too.
--
To view, visit https://review.coreboot.org/c/flashrom/+/71621
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia1cfd89a4fccfa07ba3c9ee1f6df1422ab95fc76
Gerrit-Change-Number: 71621
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Jan 2023 13:02:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71620 )
Change subject: internal.c: Add preinit hook
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
File internal.c:
https://review.coreboot.org/c/flashrom/+/71620/comment/bcf3b6c0_92038464
PS1, Line 223: ret = get_params(cfg,
: &force_boardenable, &force_boardmismatch,
: &force_laptop, ¬_a_laptop,
: &board_vendor, &board_model);
: if (ret)
: return ret;
It would be nice to have this done in pre-init as well. To do so, one would need to add a pointer to an opaque struct. The definition of the opaque struct would be private for each programmer, so each programmer would define its own struct to store the values of programmer-specific cfg options. The end goal would be to separate command-line parameter parsing from actual programmer init.
We can (and should) discuss this in a video call, e.g. dev meeting or the like.
(Marking as unresolved only because we need to agree on where/when to discuss this further)
--
To view, visit https://review.coreboot.org/c/flashrom/+/71620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9c978e4e74b07a2276f98dcab149ae24b8220c5f
Gerrit-Change-Number: 71620
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 02 Jan 2023 12:28:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment