Attention is currently required from: Anastasia Klimchuk, Brian Norris.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82175?usp=email )
Change subject: flashrom: Change chip unlock error to warning
......................................................................
Patch Set 1: Verified+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/82175?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: I14c3b55e387443909ca1efab2fc1901f87dd66d6
Gerrit-Change-Number: 82175
Gerrit-PatchSet: 1
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 03 May 2024 00:54:56 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/82175?usp=email )
Change subject: flashrom: Change chip unlock error to warning
......................................................................
flashrom: Change chip unlock error to warning
Failing to disable WP before write/erase doesn't necessarily indicate an
error and flashrom doesn't treat it as such. Print a warning instead on
an error.
BUG=b:336220545
BRANCH=none
TEST=build
Change-Id: I14c3b55e387443909ca1efab2fc1901f87dd66d6
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M flashrom.c
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/75/82175/1
diff --git a/flashrom.c b/flashrom.c
index 630c69d..39c3612 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -2092,7 +2092,7 @@
warn_out:
if (ret)
- msg_cerr("Failed to unlock flash status reg with wp support.\n");
+ msg_cwarn("Failed to unlock flash status reg with wp support.\n");
return ret;
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/82175?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: I14c3b55e387443909ca1efab2fc1901f87dd66d6
Gerrit-Change-Number: 82175
Gerrit-PatchSet: 1
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newchange
Nicholas Chin has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/82162?usp=email )
Change subject: flashrom_udev.rules: Add rule for CH347
......................................................................
flashrom_udev.rules: Add rule for CH347
This allows the CH347 programmer to be used without root permissions.
Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com>
Change-Id: Ia83fa08f6d7c2f449b1a5c0c387c6d4368b99e3a
---
M util/flashrom_udev.rules
1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/82162/1
diff --git a/util/flashrom_udev.rules b/util/flashrom_udev.rules
index 9fd3330..331fee6 100644
--- a/util/flashrom_udev.rules
+++ b/util/flashrom_udev.rules
@@ -93,4 +93,7 @@
# Winchiphead (WCH) CH341a based programmer
ATTRS{idVendor}=="1a86", ATTRS{idProduct}=="5512", MODE="664", GROUP="plugdev"
+# Winchiphead (WCH) CH347 based programmer
+ATTRS{idVendor}=="1a86", ATTRS{idProduct}=="55db", MODE="664", GROUP="plugdev"
+
LABEL="flashrom_rules_end"
--
To view, visit https://review.coreboot.org/c/flashrom/+/82162?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: Ia83fa08f6d7c2f449b1a5c0c387c6d4368b99e3a
Gerrit-Change-Number: 82162
Gerrit-PatchSet: 1
Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-MessageType: newchange
Attention is currently required from: Anastasia Klimchuk, Hsuan Ting Chen, Subrata Banik.
Hsuan-ting Chen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81357?usp=email )
Change subject: ichspi.c: Add support for region 9 and beyond in Meteor Lake
......................................................................
Patch Set 12:
(11 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/81357/comment/6d3a3826_39bbca85 :
PS10, Line 28:
> Is MTL a Meteor Lake and ADL Alder Point?
YES
Sure, let me add a test for APL. (apollo lake)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/81357/comment/5ba96866_bb53f848 :
PS10, Line 1853: mmio_readw
> Why it is `readw` (reads into uint16_t) here, but `readl` below (reads into uint32_t)? […]
You could see the document of Intel Core Ultra:
For `BIOS_BM_RAP` and `BIOS_BM_WAP`:
https://edc.intel.com/content/www/de/de/design/publications/14th-generation…
```
+-----------+--------------------------------------+
| Bit Range | Filed Name and Description |
+-----------+--------------------------------------+
| 31:16 | Reserved |
+-----------+--------------------------------------+
| 15:0 | BIOSS Master Read Access Permissions |
+-----------+--------------------------------------+
```
As only bits 15:0 are meaningful, using `mmio_readw()` (which reads 16 bits) is sufficient. (There are also a maximum of 15 regions at present.)
For `FRAP`:
https://www.intel.sg/content/dam/doc/datasheet/io-controller-hub-9-datashee…
page 827
```
+-----------+--------------------------------------+-------------+
| Bit Range | Filed Name and Description | In flashrom |
+-----------+--------------------------------------+-------------+
| 31:24 | BIOS Master Write Access Grant | ICH_BMWAG() |
+-----------+--------------------------------------+-------------+
| 23:16 | BIOS Master Read Access Grant | ICH_BMRAG() |
+-----------+--------------------------------------+-------------+
| 15:8 | BIOS Region Write Access | ICH_BRWA() |
+-----------+--------------------------------------+-------------+
| 7:0 | BIOS Region Read Access | ICH_BRRA() |
+-----------+--------------------------------------+-------------+
```
As we need all 32 bits, using `mmio_readl()` (which reads 32 bits) is necessary.
Therefore, I suggest using `region_read_access` and `region_write_access` as `uint16_t*`, rather than changing any of the current `mmio_read` functions.
https://review.coreboot.org/c/flashrom/+/81357/comment/bafbee7d_a43f666b :
PS10, Line 1854: *region_write_access = mmio_readw(ich_spibar + ICH_REG_BIOS_BM_WAP);
> Maybe you can also add some debugging prints in this branch too
Done
https://review.coreboot.org/c/flashrom/+/81357/comment/7ab01994_497a6e1e :
PS10, Line 1876: inline
> I don't think this needs to be inline?
Done
Could I know if there's any concern of inline it?
https://review.coreboot.org/c/flashrom/+/81357/comment/4467c8a7_b696bbcc :
PS10, Line 1877: if (ich_generation >= CHIPSET_METEOR_LAKE)
: return 16;
: return 8;
> maybe make it one line […]
Done
https://review.coreboot.org/c/flashrom/+/81357/comment/d08d8da3_5a41563d :
PS10, Line 2109: case CHIPSET_METEOR_LAKE:
> Let's re-order Meteor Lake here too
Done
https://review.coreboot.org/c/flashrom/+/81357/comment/3f3bbbae_7ad5fcd5 :
PS10, Line 2149: case CHIPSET_METEOR_LAKE:
> and here
Done
https://review.coreboot.org/c/flashrom/+/81357/comment/4d25f45a_f00ff889 :
PS10, Line 2211: case CHIPSET_METEOR_LAKE:
> and here
Done
https://review.coreboot.org/c/flashrom/+/81357/comment/dddfd728_c6289bb2 :
PS10, Line 2229:
> you can leave the word "registers", so that it is "... […]
Done
https://review.coreboot.org/c/flashrom/+/81357/comment/099ee78e_80fb4172 :
PS10, Line 2291: case CHIPSET_METEOR_LAKE:
> and here
Done
https://review.coreboot.org/c/flashrom/+/81357/comment/1e511f27_37e13d65 :
PS10, Line 2331: case CHIPSET_METEOR_LAKE:
> and here
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/81357?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: I1e06e7b3d470423a6014e623826d9234fdebfbf9
Gerrit-Change-Number: 81357
Gerrit-PatchSet: 12
Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-CC: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com>
Gerrit-Attention: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 02 May 2024 11:50:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Hsuan-ting Chen, Subrata Banik.
Hello Anastasia Klimchuk, Hsuan Ting Chen, Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/81357?usp=email
to look at the new patch set (#11).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: ichspi.c: Add support for region 9 and beyond in Meteor Lake
......................................................................
ichspi.c: Add support for region 9 and beyond in Meteor Lake
Since Meteor Lake, configuring region access for FREG9 and higher is
necessary. This configuration is determined using BIOS_BM registers:
BIOS_BM_RAP (Offset 0x118): BIOS Master Read Access Permissions.
Each bit [15:0] corresponds to a region [15:0].
A set bit grants BIOS master read access.
BIOS_BM_WAP (Offset 0x11c): BIOS Master Write Access Permissions.
Each bit [15:0] corresponds to a region [15:0].
A set bit grants BIOS master write/erase access.
Move CHIPSET_METEOR_LAKE to the bottom of the ich_chipset list to ensure
that all the newer chipsets in the future will use BIOS_BM check by
default.
BUG=b:319773700, b:304439294
BUG=b:319336080
TEST=On MTL, use flashrom -VV to see correct FREG9 access
TEST=On ADL, use flashrom -VV to see not break anything
TEST=On APL, use flashrom -VV to see not break anything
Change-Id: I1e06e7b3d470423a6014e623826d9234fdebfbf9
Signed-off-by: Hsuan Ting Chen <roccochen(a)chromium.org>
---
M ichspi.c
M include/programmer.h
2 files changed, 67 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/57/81357/11
--
To view, visit https://review.coreboot.org/c/flashrom/+/81357?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: I1e06e7b3d470423a6014e623826d9234fdebfbf9
Gerrit-Change-Number: 81357
Gerrit-PatchSet: 11
Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org>
Gerrit-CC: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-CC: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Anastasia Klimchuk, Angel Pons, Brian Norris, Thomas Heijligen.
Hsuan-ting Chen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82083?usp=email )
Change subject: flashrom_tester: Correct "WP screw" message
......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS5:
> Hsuan-ting, thanks for lots of information! I read all of this, I like information :) […]
1. While I initially suggested `remove` for consistency with ChromeOS documentation, upon further reflection, `open/close` seems equally valid for the WP screw issue, as it's likely a minor concern since ChromeOS has discontinued this WP screw design, and most `flashrom_tester` users are unlikely to encounter it. Ultimately, I think I would prefer todefer to Brian's decision on the terminology, as Brian is the original author and the initial feedback was positive.
2. Use a command should be the most common way to enable/disable HWWP for ChromeOS in practice but we didn't mentioned it in the previous texts. I'd like to take this opportunity to highlight this method. However, there are multiple commands available for this task. As shown in the linked documentation, three options are `gsctool`, `wp`, and `dut-control fw_wp_state`. Since listing all options would be cumbersome, I opted for the more general phrase "use the designated command".
https://chromium.googlesource.com/chromiumos/docs/+/master/write_protection…
3. Additionally, to ensure that `flashrom_tester` remains a versatile tool, it would be valuable to understand its usage beyond ChromeOS. Anastasia, based on your experience, do you have any insights into whether non-ChromeOS users are actively utilizing this tool? This information would greatly help us in making informed decisions moving forward. (This is another why I opted not to include specific commands in the debug message).
--
To view, visit https://review.coreboot.org/c/flashrom/+/82083?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: I45f06db51e92e68bf724b13bdf5b31bba511d270
Gerrit-Change-Number: 82083
Gerrit-PatchSet: 5
Gerrit-Owner: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Evan Benn <evanbenn(a)gmail.com>
Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 02 May 2024 07:09:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hsuan-ting Chen <roccochen(a)google.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, J. Neuschäfer, Riku Viitanen, Swift Geek (Sebastian Grzywna), Thomas Heijligen, Urja Rannikko.
Sydney has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82018?usp=email )
Change subject: doc: Convert serprog docs to rst and add to doc directory
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/82018?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: Ie52f1e051ed215d61d5fb535e3eddeac71f64d13
Gerrit-Change-Number: 82018
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Riku Viitanen <riku.viitanen(a)protonmail.com>
Gerrit-Reviewer: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com>
Gerrit-Reviewer: Sydney <git(a)funkeleinhorn.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: J. Neuschäfer <j.neuschaefer(a)gmx.net>
Gerrit-Attention: Riku Viitanen <riku.viitanen(a)protonmail.com>
Gerrit-Attention: Urja Rannikko <urjaman(a)gmail.com>
Gerrit-Attention: J. Neuschäfer <j.neuschaefer(a)gmx.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com>
Gerrit-Comment-Date: Wed, 01 May 2024 17:52:37 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Brian Norris, Hsuan-ting Chen, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82083?usp=email )
Change subject: flashrom_tester: Correct "WP screw" message
......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS5:
> More references here: […]
Hsuan-ting, thanks for lots of information! I read all of this, I like information :)
As for me, I am fine with "open / close" or also would be fine with "remove / insert" (I think "restore" is confusing). I also don't think we need "use the designated command", which command?
Out of people on the patch, Brian and Hsuan-ting are currently active users of flashrom_tester.
Brian, what do you think? Let's decide.
"open / close" or "remove / insert" ?
3 people who are not currently active users of flashrom_tester agree with "open / close" :)
--
To view, visit https://review.coreboot.org/c/flashrom/+/82083?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: I45f06db51e92e68bf724b13bdf5b31bba511d270
Gerrit-Change-Number: 82083
Gerrit-PatchSet: 5
Gerrit-Owner: Brian Norris <briannorris(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Evan Benn <evanbenn(a)gmail.com>
Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 01 May 2024 11:02:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hsuan-ting Chen <roccochen(a)google.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: J. Neuschäfer, Riku Viitanen, Swift Geek (Sebastian Grzywna), Sydney, Thomas Heijligen, Urja Rannikko.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82018?usp=email )
Change subject: doc: Convert serprog docs to rst and add to doc directory
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
I wanted to ask, if people have no more comments and in general agree with the patch, could you vote +1 ? Thank you!
--
To view, visit https://review.coreboot.org/c/flashrom/+/82018?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: Ie52f1e051ed215d61d5fb535e3eddeac71f64d13
Gerrit-Change-Number: 82018
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Riku Viitanen <riku.viitanen(a)protonmail.com>
Gerrit-Reviewer: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com>
Gerrit-Reviewer: Sydney <git(a)funkeleinhorn.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: J. Neuschäfer <j.neuschaefer(a)gmx.net>
Gerrit-Attention: Riku Viitanen <riku.viitanen(a)protonmail.com>
Gerrit-Attention: Urja Rannikko <urjaman(a)gmail.com>
Gerrit-Attention: J. Neuschäfer <j.neuschaefer(a)gmx.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Sydney <git(a)funkeleinhorn.com>
Gerrit-Attention: Swift Geek (Sebastian Grzywna) <swiftgeek(a)gmail.com>
Gerrit-Comment-Date: Wed, 01 May 2024 10:52:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment