Attention is currently required from: Alexander Goncharov, Anastasia Klimchuk, Peter Marheine.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78689?usp=email )
Change subject: doc: Add meson test command to TLDR for meson instructions
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
Patchset:
PS1:
> That's a good idea, and I forgot about TLDR in README. I added there too. […]
I would agree that when developers see the sequence "setup", "compile", "test", "install". They can figure out that if they do not need the binaries in their /usr/local/bin or wherever, they will skip the final "install" command, or if they to not care about running tests (on un-modified source code maybe), then they will skip the "test" step. I do not think it is necessary to explicitly call out either as being optional.
--
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: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Jes Klinke <jbk(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Mon, 30 Oct 2023 16:28:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net>
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 (#6).
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
M tests/tests.c
M tests/tests.h
5 files changed, 132 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/99/77999/6
--
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: 6
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
Attention is currently required from: Alexander Goncharov, Peter Marheine.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78689?usp=email )
Change subject: doc: Add meson test command to TLDR for meson instructions
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
> Many users just want to compile things and don't care about running the tests, so I think it's reaso […]
That's a good idea, and I forgot about TLDR in README. I added there too.
About marking optional: I thought about it for a bit. There is currently a `meson install` command in both docs, which is also optional and not everyone needs it.
Peter, do you think we should mark both as optional?
I have the thought of, maybe people would understand that if they just need to compile than they just run compile, and if they need tests they run tests?
--
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: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Jes Klinke <jbk(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Mon, 30 Oct 2023 09:35:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk.
Hello Alexander Goncharov, Jes Klinke, Peter Marheine, Thomas Heijligen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/78689?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
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.
The patch adds test command to TLDR version in README file, and
in dedicated meson instructions doc.
Change-Id: I67d5f4decdac15e6a72f4372135dab7d44396594
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
---
M README.rst
M doc/dev_guide/building_from_source.rst
2 files changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/78689/2
--
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: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Jes Klinke <jbk(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer.
Hsuan-ting Chen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78348?usp=email )
Change subject: flashchips: Split GD25Q127C/GD25Q128C and add GD25Q128E
......................................................................
Patch Set 3:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/78348/comment/27bcd5ca_403a16fe :
PS1, Line 6802: /* FIXME: 128E's OTP: 3072B total and doesn't support QPI */
> 1. Yes, I think you need to separate the two chip definitions. […]
Sure, then let's go for `128C` and `127C/128E`.
Thanks for the clarification.
I saw the comment shows `can be distinguished by SFDP` does that mean `flashrom` is able to distinguish them? Or is it impossible and we still need to add `-c` manually?
--
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: 3
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: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Mon, 30 Oct 2023 08:03:44 +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: Hsuan-ting Chen, Nikolai Artemiev, Stefan Reinauer.
Hello Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/78348?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: flashchips: Split GD25Q127C/GD25Q128C and add GD25Q128E
......................................................................
flashchips: Split GD25Q127C/GD25Q128C and add GD25Q128E
Q127C and Q128C are not the same. Q127C doesn't support QPI but Q128C
does. So we need to split the existing GD25Q127C/GD25Q128C into two
separated entries. We also introduce the new flashchip Q128E and merge
it into Q127C.
Datasheet for Q128E: https://www.gigadevice.com.cn/Public/Uploads/uploadfile/files/20220714/DS-0…
Q128E and Q127C/Q128C have compatible main functions, their differences
are:
* Q128E uses 55 nm process, while Q127C/Q128C use 65nm
* Q128E/Q127C does not support QPI
* Q128E have OTP: 3072B, while Q127C/Q128C are 1536B
* Q128E's fast read clock frequency is 133MHz, while Q127C/Q128C are
104MHZ
So we decided to merge Q128E into Q127C.
We also tested that Q128E could pass flashrom_tester while probing it as
127C/128C, so the main functionalities are compatible.
Change the chip name from GD25Q127C/GD25Q128C to two entries
GD25Q127C/GD25Q128E and GD25Q128C to make it more accurate.
Chip revision history:
- The 'GD25Q127C/GD25Q128C' definition was added in
`commit e0c7abf219b81ad049d09a4671ebc9196153d308` as 'GD25Q128C' and
later renamed to 'GD25Q127C/GD25Q128C'
BUG=b:304863141, b:293545382
BRANCH=none
TEST=flashrom_tester with flashrom binary could pass with Q128E,
which contains probe, read, write, erase, and write protect
Signed-off-by: Hsuan Ting Chen <roccochen(a)google.com>
Change-Id: I3300671b1cf74b3ea0469b9c5a833489ab4914f5
---
M flashchips.c
M include/flashchips.h
2 files changed, 52 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/48/78348/3
--
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: 3
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-MessageType: newpatchset
Attention is currently required from: Anastasia Klimchuk.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78689?usp=email )
Change subject: doc: Add meson test command to TLDR for meson instructions
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> I am sorry. […]
Many users just want to compile things and don't care about running the tests, so I think it's reasonable to omit from the main page. It would be okay to include as well, though I'd like to call tests out as optional since they're not meaningful to most users.
--
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-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Jes Klinke <jbk(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sun, 29 Oct 2023 22:24:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Anton Samsonov, Anton Samsonov, Thomas Heijligen.
Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77089?usp=email )
Change subject: Remove dependency on C23 __has_include()
......................................................................
Patch Set 3:
(3 comments)
Patchset:
PS1:
> I totally agree with all your and Alexander's propositions, however have no knowledge of how to test […]
Well done, thank you!
Please try using `cc.check_header` in Meson to check whether the specified header is usable. You can find an example by searching for `cc.check_header('sys/utsname.h')`. I assume that all tests will then be verified by Jenkins.
File Makefile:
https://review.coreboot.org/c/flashrom/+/77089/comment/5243b176_7772b912 :
PS3, Line 209:
drop extra line
https://review.coreboot.org/c/flashrom/+/77089/comment/b87050e5_3eb13c3b :
PS3, Line 240:
drop extra line
--
To view, visit https://review.coreboot.org/c/flashrom/+/77089?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: Ic544963ffd29626ae0a21bdddb1c78850cc43ec6
Gerrit-Change-Number: 77089
Gerrit-PatchSet: 3
Gerrit-Owner: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
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: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Comment-Date: Sun, 29 Oct 2023 20:01:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anton Samsonov <avscomputing(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk.
Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78689?usp=email )
Change subject: doc: Add meson test command to TLDR for meson instructions
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> There's also TLDR section in `README. […]
I am sorry. I created it as a resolved comment, but it is not => make unresolved
--
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-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Jes Klinke <jbk(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sun, 29 Oct 2023 19:22:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: comment