Attention is currently required from: Anastasia Klimchuk, Edward O'Callaghan, Nikolai Artemiev, Thomas Heijligen.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77999?usp=email )
Change subject: raiden: Support target index with generic REQ_ENABLE
......................................................................
Patch Set 5:
(2 comments)
Patchset:
PS2:
> Yes, I looked at the test now (tests/raiden_debug_spi.c), this parameter is not set. […]
I have added a few more cases to the test. In particular, I have noticed that if I put something like "target=abc" in the `raiden_debug_param` instead of the valid values such as "ap", "ec" or numbers, then the test will fail.
File doc/classic_cli_manpage.rst:
https://review.coreboot.org/c/flashrom/+/77999/comment/c587c7d9_16bd314a :
PS4, Line 873:
> There is a lot of information, I looked at your commit message and I really like the paragraph that […]
I have attempted to merge your suggested paragraph into the existing documentation.
--
To view, visit https://review.coreboot.org/c/flashrom/+/77999?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I03bf4f3210186fb5937b42e298761907b03e08b7
Gerrit-Change-Number: 77999
Gerrit-PatchSet: 5
Gerrit-Owner: Jes Klinke <jbk(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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Fri, 27 Oct 2023 21:04:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jes Klinke <jbk(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Edward O'Callaghan, Jes Klinke, Nikolai Artemiev, Thomas Heijligen.
Hello Anastasia Klimchuk, Edward O'Callaghan, Nikolai Artemiev, Thomas Heijligen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/77999?usp=email
to look at the new patch set (#5).
The following approvals got outdated and were removed:
Code-Review+1 by Anastasia Klimchuk, Verified+1 by build bot (Jenkins)
Change subject: raiden: Support target index with generic REQ_ENABLE
......................................................................
raiden: Support target index with generic REQ_ENABLE
Some devices such as the GSC knows how it is wired to AP and EC flash
chips, and can be told which specific device to talk to. Other devices
such as Servo Micro and HyperDebug are generic, and do not know how they
are wired, the caller is responsible for first configure the appropriate
MUXes or buffers, and then tell the debugger which port to use (Servo
Micro has just one SPI port, HyperDebug is the first that has multiple).
The Raiden protocol allows both the cases of USB devices knowing their
wiring and not.
If I were to declare the protocol in Rust, this is how the information
of the Raiden protocol "enable request" would be encoded:
```
enum {
EnableGeneric(u8),
EnableAp,
EnableEc,
...
}
```
The first label `EnableGeneric(u8)` is to be used with HyperDebug that
does not know how its ports are wired, and allow access by index.
The other labels `EnableAp` and `EnableEc` are to be used with the GSC.
The actual transmission of the enum above uses the bRequest and low byte
of wValue of a USB control request, but that is a detail and not
conceptually important.
Until now, `-p raiden_debug_spi:target=AP` or `...:target=EC` could be
used to make flashrom use `EnableAp` or `EnableEc`, and if neither was
given, it would default to `EnableGeneric`, which now that wValue is
used means `EnableGeneric(0)`.
I find it rather straight-forward, that `-p raiden_debug_spi:target=1`,
`...:target=2`, etc. should translate to `EnableGeneric(1)`, etc.
This patchset achieves this, by adding a second 16-bit parameter value,
next to request_enable.
I have tested that flashrom can detect the same Winbond flash chip
"W25Q128.V..M" with two different Raiden USB devices as below.
TEST=flashrom -p raiden_debug_spi:serial=0701B044-91AC3132,target=AP
TEST=flashrom -p raiden_debug_spi:serial=205635783236,target=1
Signed-off-by: Jes B. Klinke <jbk(a)chromium.org>
Change-Id: I03bf4f3210186fb5937b42e298761907b03e08b7
---
M doc/classic_cli_manpage.rst
M raiden_debug_spi.c
M tests/raiden_debug_spi.c
3 files changed, 52 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/99/77999/5
--
To view, visit https://review.coreboot.org/c/flashrom/+/77999?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I03bf4f3210186fb5937b42e298761907b03e08b7
Gerrit-Change-Number: 77999
Gerrit-PatchSet: 5
Gerrit-Owner: Jes Klinke <jbk(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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Jes Klinke <jbk(a)chromium.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/78689?usp=email )
Change subject: doc: Add meson test command to TLDR for meson instructions
......................................................................
doc: Add meson test command to TLDR for meson instructions
Running tests is one of the regular things to do in dev process,
same as compile, so it should be highlighted in the TLDR section
of the document.
Change-Id: I67d5f4decdac15e6a72f4372135dab7d44396594
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M doc/dev_guide/building_from_source.rst
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/78689/1
diff --git a/doc/dev_guide/building_from_source.rst b/doc/dev_guide/building_from_source.rst
index 64250dd..4e50753 100644
--- a/doc/dev_guide/building_from_source.rst
+++ b/doc/dev_guide/building_from_source.rst
@@ -33,6 +33,7 @@
meson setup builddir
meson compile -C builddir
+ meson test -C builddir
meson install -C builddir
--
To view, visit https://review.coreboot.org/c/flashrom/+/78689?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I67d5f4decdac15e6a72f4372135dab7d44396594
Gerrit-Change-Number: 78689
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newchange
Attention is currently required from: Edward O'Callaghan, Jes Klinke, Nikolai Artemiev, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77999?usp=email )
Change subject: raiden: Support target index with generic REQ_ENABLE
......................................................................
Patch Set 4: Code-Review+1
(2 comments)
Patchset:
PS2:
> I do not see any mention of existing values `target=AP` or `target=EC` in `raiden_debug_spi. […]
Yes, I looked at the test now (tests/raiden_debug_spi.c), this parameter is not set. Which means it goes by default flow which is, from the code... request_enable = RAIDEN_DEBUG_SPI_REQ_ENABLE.
I am not sure whether this is realistic (or even valid) scenario, but because unit tests do not talk to actual hardware, only emulate it, this can explain why unit test passes.
For some more content, unit tests are relatively recent in comparison with the most of code base (started in 2021 vs ~2000), so there is of course lots of room for improvement and adding more coverage.
How to run tests: instructions here https://flashrom.org/dev_guide/building_from_source.html#unit-tests
If you don't have meson build dir yet, you need to scroll up in the doc. tldr
mkdir builddir
meson setup builddir
meson compile -C builddir
meson test -C builddir
Also you can run `ninja test` inside builddir once you setup.
For adding new test:
You can copy `raiden_debug_basic_lifecycle_test_success` and add target param into `raiden_debug_param` string. Perhaps all the mocks will apply without any change (I would try)!
You need to add your new test function into tests/tests.c and tests/tests.h
If new test run, that's amazing! If it fails, or does not run, you can still push new patchset and tell what the error is.
It is possible that maybe some of the mock functions (you can see them in `raiden_debug_io` need to be copied->adjusted) but first lets see whether test runs.
I am plannig to write a doc "How to add unit test", I will use this my comment as a starting point :)
File doc/classic_cli_manpage.rst:
https://review.coreboot.org/c/flashrom/+/77999/comment/2bd2c13a_7aa14914 :
PS4, Line 873:
There is a lot of information, I looked at your commit message and I really like the paragraph that you wrote. Can you copy it here? (and keep it in commit message of course).
The paragraph is easy to read, and it explains stuff, also importantly it gives examples of devices.
Let's inject this on the line 873 ?
> Some devices such as the GSC knows how it is wired to AP and EC flash
chips, and can be told which specific device to talk to. Other devices
such as Servo Micro and HyperDebug are generic, and do not know how they
are wired, the caller is responsible for first configure the appropriate
MUXes or buffers, and then tell the debugger which port to use (Servo
Micro has just one SPI port, HyperDebug is the first that has multiple).
The Raiden protocol allows both the cases of USB devices knowing their
wiring and not.
--
To view, visit https://review.coreboot.org/c/flashrom/+/77999?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I03bf4f3210186fb5937b42e298761907b03e08b7
Gerrit-Change-Number: 77999
Gerrit-PatchSet: 4
Gerrit-Owner: Jes Klinke <jbk(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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Jes Klinke <jbk(a)chromium.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Fri, 27 Oct 2023 06:21:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Jes Klinke <jbk(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Hsuan-ting Chen, Nikolai Artemiev, Stefan Reinauer.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78348?usp=email )
Change subject: flashchips: Add GD25Q128E name to the GD25Q127C/GD25Q128C entry
......................................................................
Patch Set 2:
(2 comments)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/78348/comment/93f859e3_7b14fbbd :
PS1, Line 6802: /* FIXME: 128E's OTP: 3072B total and doesn't support QPI */
> I have two questions about these: […]
1. Yes, I think you need to separate the two chip definitions. This is what I meant when I said you need to split. You can copypaste existing definition into new one, make necessary changes and adjustments, and put new the one in alphabetical order.
At the end there will be two definitions instead of one that it now.
Also you will need to update the commit title.
2. If there are 3 different chips with the same IDs, you will need to provide chip name with `-c` or `--chip` options (you can check this option on man page which is also on the web page https://flashrom.org/classic_cli_manpage.html#options)
https://review.coreboot.org/c/flashrom/+/78348/comment/78a3141c_58c89996 :
PS1, Line 6833: .reg_bits =
: {
: .srp = {STATUS1, 7, RW},
: .srl = {STATUS2, 0, RW},
: .bp = {{STATUS1, 2, RW}, {STATUS1, 3, RW}, {STATUS1, 4, RW}},
: .tb = {STATUS1, 5, RW}, /* Called BP3 in datasheet, acts like TB */
: .sec = {STATUS1, 6, RW}, /* Called BP4 in datasheet, acts like SEC */
: .cmp = {STATUS2, 6, RW},
: }
> > Are all these registers the same for new chip you are adding […]
Great thank you!
--
To view, visit https://review.coreboot.org/c/flashrom/+/78348?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I3300671b1cf74b3ea0469b9c5a833489ab4914f5
Gerrit-Change-Number: 78348
Gerrit-PatchSet: 2
Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Fri, 27 Oct 2023 05:19:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hsuan-ting Chen <roccochen(a)google.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Edward O'Callaghan, Nikolai Artemiev, Thomas Heijligen.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77999?usp=email )
Change subject: raiden: Support target index with generic REQ_ENABLE
......................................................................
Patch Set 4:
(7 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/77999/comment/1536cdca_86be7fba :
PS2, Line 42: target=1
> Since these are new values for `target` param, please document it on the manpage, here: […]
Done
https://review.coreboot.org/c/flashrom/+/77999/comment/1cc81745_67b2379c :
PS2, Line 45: CL
> Just noticed: you need to say "this patch" […]
Done
https://review.coreboot.org/c/flashrom/+/77999/comment/97759e44_e57e1e0d :
PS2, Line 45: archieves
> achieves (typo)
Done
https://review.coreboot.org/c/flashrom/+/77999/comment/dca9dc69_122661b9 :
PS2, Line 48: run flashrom with HyperDebug
> Could you please list the exact commands that you ran? […]
Done
Patchset:
PS2:
> Also, maybe you can add a test for target=1 and 2? […]
I do not see any mention of existing values `target=AP` or `target=EC` in `raiden_debug_spi.c`, I think I will need more help on how to create a test for this.
How do I even run the existing test of the Raiden SPI protocol?
File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/77999/comment/ff2cc291_f4cdb339 :
PS2, Line 1596: request_enable & 0xFF
> If this is a no-op, and given your idea to change the signature of `get_target` , maybe this is not […]
Acknowledged
https://review.coreboot.org/c/flashrom/+/77999/comment/58b39cce_12478b9d :
PS2, Line 1596: request_enable & 0xFF,
: (request_enable >> 8) & 0xFF,
> Jes, thank you for detailed explanation, I understand much better now! […]
The documentation at the start of the file actually mentions only the bulk data on the USB endpoint, not the initial control transfer extended by this patchset. So all if it still applies.
--
To view, visit https://review.coreboot.org/c/flashrom/+/77999?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I03bf4f3210186fb5937b42e298761907b03e08b7
Gerrit-Change-Number: 77999
Gerrit-PatchSet: 4
Gerrit-Owner: Jes Klinke <jbk(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
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: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 25 Oct 2023 20:25:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jes Klinke <jbk(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Jes Klinke, Nikolai Artemiev.
Hello Edward O'Callaghan, Nikolai Artemiev, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/77999?usp=email
to look at the new patch set (#4).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: raiden: Support target index with generic REQ_ENABLE
......................................................................
raiden: Support target index with generic REQ_ENABLE
Some devices such as the GSC knows how it is wired to AP and EC flash
chips, and can be told which specific device to talk to. Other devices
such as Servo Micro and HyperDebug are generic, and do not know how they
are wired, the caller is responsible for first configure the appropriate
MUXes or buffers, and then tell the debugger which port to use (Servo
Micro has just one SPI port, HyperDebug is the first that has multiple).
The Raiden protocol allows both the cases of USB devices knowing their
wiring and not.
If I were to declare the protocol in Rust, this is how the information
of the Raiden protocol "enable request" would be encoded:
```
enum {
EnableGeneric(u8),
EnableAp,
EnableEc,
...
}
```
The first label `EnableGeneric(u8)` is to be used with HyperDebug that
does not know how its ports are wired, and allow access by index.
The other labels `EnableAp` and `EnableEc` are to be used with the GSC.
The actual transmission of the enum above uses the bRequest and low byte
of wValue of a USB control request, but that is a detail and not
conceptually important.
Until now, `-p raiden_debug_spi:target=AP` or `...:target=EC` could be
used to make flashrom use `EnableAp` or `EnableEc`, and if neither was
given, it would default to `EnableGeneric`, which now that wValue is
used means `EnableGeneric(0)`.
I find it rather straight-forward, that `-p raiden_debug_spi:target=1`,
`...:target=2`, etc. should translate to `EnableGeneric(1)`, etc.
This patchset achieves this, by adding a second 16-bit parameter value,
next to request_enable.
I have tested that flashrom can detect the same Winbond flash chip
"W25Q128.V..M" with two different Raiden USB devices as below.
TEST=flashrom -p raiden_debug_spi:serial=0701B044-91AC3132,target=AP
TEST=flashrom -p raiden_debug_spi:serial=205635783236,target=1
Signed-off-by: Jes B. Klinke <jbk(a)chromium.org>
Change-Id: I03bf4f3210186fb5937b42e298761907b03e08b7
---
M doc/classic_cli_manpage.rst
M raiden_debug_spi.c
2 files changed, 25 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/99/77999/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/77999?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I03bf4f3210186fb5937b42e298761907b03e08b7
Gerrit-Change-Number: 77999
Gerrit-PatchSet: 4
Gerrit-Owner: Jes Klinke <jbk(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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Jes Klinke <jbk(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/78186?usp=email )
(
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: ichspi: Add support for C740 PCH
......................................................................
ichspi: Add support for C740 PCH
Clean commit 51e1d0e4b7670e5822560acc724a6a8dd00b6af4
'Add support for Intel Emmitsburg PCH' which broke
CHIPSET_5_SERIES_IBEX_PEAK detection and which assumes C740 is the same
as C620, while its more a close relative to Intel's H570 PCH.
Based on Intel SPI Programming Guide #619386.
Test: Run on Intel ArcherCity CRB with Intel's C741 PCH
using the 'internal' programmer.
Test: Run on BMC and accessed the SPI flash chip over
'linux_mtd' programmer.
Change-Id: I80eebc0fcc14de9df823aceaee77870ad136f94a
Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/78186
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M chipset_enable.c
M ich_descriptors.c
M ichspi.c
M include/programmer.h
4 files changed, 46 insertions(+), 8 deletions(-)
Approvals:
build bot (Jenkins): Verified
Anastasia Klimchuk: Looks good to me, approved
diff --git a/chipset_enable.c b/chipset_enable.c
index 3846ecf..1c24b92 100644
--- a/chipset_enable.c
+++ b/chipset_enable.c
@@ -601,6 +601,7 @@
break;
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -712,6 +713,7 @@
break;
case CHIPSET_500_SERIES_TIGER_POINT:
case CHIPSET_600_SERIES_ALDER_POINT:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_METEOR_LAKE:
boot_straps = boot_straps_pch500;
break;
@@ -987,6 +989,11 @@
return enable_flash_pch100_or_c620(cfg, dev, name, 0x1f, 5, CHIPSET_C620_SERIES_LEWISBURG);
}
+static int enable_flash_c740(const struct programmer_cfg *cfg, struct pci_dev *const dev, const char *const name)
+{
+ return enable_flash_pch100_or_c620(cfg, dev, name, 0x1f, 5, CHIPSET_C740_SERIES_EMMITSBURG);
+}
+
static int enable_flash_pch300(const struct programmer_cfg *cfg, struct pci_dev *const dev, const char *const name)
{
return enable_flash_pch100_or_c620(cfg, dev, name, 0x1f, 5, CHIPSET_300_SERIES_CANNON_POINT);
@@ -2123,7 +2130,7 @@
{0x8086, 0xa247, B_S, NT, "Intel", "C620 Series Chipset Supersku", enable_flash_c620},
{0x8086, 0xa248, B_S, NT, "Intel", "C620 Series Chipset Supersku", enable_flash_c620},
{0x8086, 0xa249, B_S, NT, "Intel", "C620 Series Chipset Supersku", enable_flash_c620},
- {0x8086, 0x1bca, B_S, NT, "Intel", "Emmitsburg Chipset SKU", enable_flash_c620},
+ {0x8086, 0x1bca, B_S, DEP, "Intel", "Emmitsburg Chipset SKU", enable_flash_c740},
{0x8086, 0xa2c4, B_S, NT, "Intel", "H270", enable_flash_pch100},
{0x8086, 0xa2c5, B_S, NT, "Intel", "Z270", enable_flash_pch100},
{0x8086, 0xa2c6, B_S, NT, "Intel", "Q270", enable_flash_pch100},
diff --git a/ich_descriptors.c b/ich_descriptors.c
index c3d062e..7bdca61 100644
--- a/ich_descriptors.c
+++ b/ich_descriptors.c
@@ -44,6 +44,7 @@
case CHIPSET_GEMINI_LAKE:
return 6;
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -75,6 +76,7 @@
{
switch (cs) {
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_APOLLO_LAKE:
case CHIPSET_600_SERIES_ALDER_POINT:
case CHIPSET_METEOR_LAKE:
@@ -117,9 +119,9 @@
"5 series Ibex Peak", "6 series Cougar Point", "7 series Panther Point",
"8 series Lynx Point", "Baytrail", "8 series Lynx Point LP", "8 series Wellsburg",
"9 series Wildcat Point", "9 series Wildcat Point LP", "100 series Sunrise Point",
- "C620 series Lewisburg", "300 series Cannon Point", "400 series Comet Point",
- "500 series Tiger Point", "600 series Alder Point", "Meteor Lake",
- "Apollo Lake", "Gemini Lake", "Jasper Lake", "Elkhart Lake",
+ "C620 series Lewisburg", "C740 series Emmitsburg", "300 series Cannon Point",
+ "400 series Comet Point", "500 series Tiger Point", "600 series Alder Point",
+ "Meteor Lake", "Apollo Lake", "Gemini Lake", "Jasper Lake", "Elkhart Lake",
};
if (cs < CHIPSET_ICH8 || cs - CHIPSET_ICH8 + 1 >= ARRAY_SIZE(chipset_names))
cs = 0;
@@ -211,6 +213,7 @@
case CHIPSET_9_SERIES_WILDCAT_POINT_LP:
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -313,6 +316,7 @@
return freq_str[2][value];
case CHIPSET_500_SERIES_TIGER_POINT:
case CHIPSET_600_SERIES_ALDER_POINT:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_METEOR_LAKE:
return freq_str[3][value];
case CHIPSET_ELKHART_LAKE:
@@ -357,6 +361,7 @@
switch (cs) {
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -472,15 +477,27 @@
void prettyprint_ich_descriptor_master(const enum ich_chipset cs, const struct ich_descriptors *const desc)
{
ssize_t i;
- const ssize_t nm = ich_number_of_masters(cs, &desc->content);
+ ssize_t nm = ich_number_of_masters(cs, &desc->content);
msg_pdbg2("=== Master Section ===\n");
if (nm < 0) {
msg_pdbg2("%s: number of masters too high (%d).\n", __func__,
desc->content.NM + 1);
return;
}
+ if (cs == CHIPSET_C740_SERIES_EMMITSBURG) {
+ /*
+ * The SPI programming guide says there are 6 masters (thus NM=6), but it
+ * can only name 5. If there's a 6th then it's undocumented.
+ * However the first 5 are matching '500 Series PCH' and since C740 is a
+ * 500 Series clone, this field probably was not updated when writing the
+ * document.
+ * Hardcode to 5 to be compatible with '500 Series PCH' below.
+ */
+ nm = 5;
+ }
for (i = 0; i < nm; i++)
msg_pdbg2("FLMSTR%zd 0x%08"PRIx32"\n", i + 1, desc->master.FLMSTRs[i]);
+
msg_pdbg2("\n");
msg_pdbg2("--- Details ---\n");
@@ -489,6 +506,7 @@
cs == CHIPSET_400_SERIES_COMET_POINT ||
cs == CHIPSET_500_SERIES_TIGER_POINT ||
cs == CHIPSET_600_SERIES_ALDER_POINT ||
+ cs == CHIPSET_C740_SERIES_EMMITSBURG ||
cs == CHIPSET_JASPER_LAKE || cs == CHIPSET_METEOR_LAKE) {
const char *const master_names[] = {
"BIOS", "ME", "GbE", "DevE", "EC",
@@ -1021,8 +1039,8 @@
warn_peculiar_desc("Gemini Lake");
return CHIPSET_GEMINI_LAKE;
}
- if (content->ISL <= 80)
- return CHIPSET_C620_SERIES_LEWISBURG;
+ if (content->ISL == 0x50)
+ return CHIPSET_C740_SERIES_EMMITSBURG;
warn_peculiar_desc("Ibex Peak");
return CHIPSET_5_SERIES_IBEX_PEAK;
} else if (upper->MDTBA == 0x00) {
@@ -1095,6 +1113,7 @@
break;
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_APOLLO_LAKE:
if (component->modes.freq_read != 6) {
msg_pwarn("\nThe flash descriptor looks like a Skylake/Sunrise Point descriptor.\n"
@@ -1242,6 +1261,7 @@
case CHIPSET_9_SERIES_WILDCAT_POINT_LP:
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -1287,6 +1307,7 @@
switch (cs) {
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
diff --git a/ichspi.c b/ichspi.c
index 2e38ff1..36a7b3b 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -457,6 +457,7 @@
switch (ich_gen) {
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -470,6 +471,7 @@
switch (ich_gen) {
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -492,6 +494,7 @@
switch (ich_gen) {
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -2050,6 +2053,7 @@
switch (ich_gen) {
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -2087,6 +2091,7 @@
*num_freg = 10;
break;
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
*num_freg = 12; /* 12 MMIO regs, but 16 regions in FD spec */
break;
case CHIPSET_300_SERIES_CANNON_POINT:
@@ -2150,6 +2155,7 @@
switch (ich_gen) {
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -2230,6 +2236,7 @@
case CHIPSET_ICH8:
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -2269,6 +2276,7 @@
case CHIPSET_ICH8:
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
+ case CHIPSET_C740_SERIES_EMMITSBURG:
case CHIPSET_300_SERIES_CANNON_POINT:
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_500_SERIES_TIGER_POINT:
@@ -2310,7 +2318,8 @@
ich_gen == CHIPSET_300_SERIES_CANNON_POINT ||
ich_gen == CHIPSET_400_SERIES_COMET_POINT ||
ich_gen == CHIPSET_500_SERIES_TIGER_POINT ||
- ich_gen == CHIPSET_600_SERIES_ALDER_POINT)) {
+ ich_gen == CHIPSET_600_SERIES_ALDER_POINT ||
+ ich_gen == CHIPSET_C740_SERIES_EMMITSBURG)) {
msg_pdbg("Enabling hardware sequencing by default for 100+ series PCH.\n");
ich_spi_mode = ich_hwseq;
}
diff --git a/include/programmer.h b/include/programmer.h
index 562f58a..15df0d4 100644
--- a/include/programmer.h
+++ b/include/programmer.h
@@ -354,6 +354,7 @@
CHIPSET_9_SERIES_WILDCAT_POINT_LP,
CHIPSET_100_SERIES_SUNRISE_POINT, /* also 6th/7th gen Core i/o (LP) variants */
CHIPSET_C620_SERIES_LEWISBURG,
+ CHIPSET_C740_SERIES_EMMITSBURG,
CHIPSET_300_SERIES_CANNON_POINT,
CHIPSET_400_SERIES_COMET_POINT,
CHIPSET_500_SERIES_TIGER_POINT,
--
To view, visit https://review.coreboot.org/c/flashrom/+/78186?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I80eebc0fcc14de9df823aceaee77870ad136f94a
Gerrit-Change-Number: 78186
Gerrit-PatchSet: 7
Gerrit-Owner: Patrick Rudolph <rudolphpatrick05(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-MessageType: merged