Attention is currently required from: DZ, Nikolai Artemiev, Stefan Reinauer.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81563?usp=email )
Change subject: flashchips: Add support for MXIC MX25L3239E
......................................................................
Patch Set 6:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/81563/comment/169a9945_cede384b :
PS3, Line 9523: 1024B
> I found it is "4K-bit secured OTP" in datasheet, so maybe twice less?
Daniel, I noticed on this patch (and few others that you currently have under review), you left unresolved comments, why? I see that you fixed what the comment was asking, but did not resolve comment as Done.
For the reviewers (me, in this case), resolving comment as Done indicates that you are happy with the latest version of your patch, and reviewers can have another look.
If the comment is not resolved as Done, reviewers are not sure, maybe you are still working on the patch, or testing it, so it's not ready yet.
If this patch is ready (and same for the others), you need to click Done on each comment. And then you need to Reply to the patch, there is a button on the top. Then you click Send, and after that all reviewers get an email, and this is an indication that the patch is ready.
Thank you!
--
To view, visit https://review.coreboot.org/c/flashrom/+/81563?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: Ic7a848028fe937deb1bf83ef2a9dddf1330334b6
Gerrit-Change-Number: 81563
Gerrit-PatchSet: 6
Gerrit-Owner: DZ <danielzhang(a)mxic.com.cn>
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: DZ <danielzhang(a)mxic.com.cn>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 24 Apr 2024 08:07:19 +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: Brian Norris, Nikolai Artemiev, Peter Marheine.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80807?usp=email )
Change subject: flashrom: Don't throw around "delay 1 second" so lightly
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS1:
I just thought of something and decided to check...
Erase operation verifies the chip straight away after erasing. Without any delay. (Also without respecting the noverify flags, but this we will fix later).
From the chip point of view, erase is almost the same as write, is that correct?
So maybe the erase flow is kind of a "proof-of-concept" for what this patch is doing?
I completely forgot that write and verify can be separated in a manual workflow, so silly of me. This changes the situation, maybe we don't need a special option.
> do you want me to start such a mailing list thread, or were you suggesting I do this?
Brian, you gave me two options which are the same :) But I agree with both of them. I think you should start the thread.
This was your idea, your work and you even went through 10yr old commit messages!
Add a link to the patch in the post, so that anyone who is interested can easily click.
--
To view, visit https://review.coreboot.org/c/flashrom/+/80807?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: Ie09651fede3f9f03425244c94a2da8bae00315fc
Gerrit-Change-Number: 80807
Gerrit-PatchSet: 3
Gerrit-Owner: Brian Norris <briannorris(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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Tue, 23 Apr 2024 09:10:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Brian Norris.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80807?usp=email )
Change subject: flashrom: Don't throw around "delay 1 second" so lightly
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS1:
> I'd avoid adding a flag - if there really are chips that need a delay, then there is a hardware or s […]
Agree that since a manual workflow can do the same thing, we shouldn't add an option. I think that recommendation should be captured in some kind of changelog so there's "real" documentation of the change, though.
--
To view, visit https://review.coreboot.org/c/flashrom/+/80807?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: Ie09651fede3f9f03425244c94a2da8bae00315fc
Gerrit-Change-Number: 80807
Gerrit-PatchSet: 3
Gerrit-Owner: Brian Norris <briannorris(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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 23 Apr 2024 03:24:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Brian Norris, Peter Marheine.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80807?usp=email )
Change subject: flashrom: Don't throw around "delay 1 second" so lightly
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS1:
> Unfortunately, this hack survived so long it migrated from the CLI to the library. […]
I'd avoid adding a flag - if there really are chips that need a delay, then there is a hardware or software bug that should be addressed with a chip flag or driver fix instead. Adding a flag makes a temporary fix more permanent.
If users need to flash a buggy chip and can't wait for a new flashrom version that fixes it, they can always separate the write & verify operations as Brian suggests.
A mailing list post seems like a good option for trying to recover the history around why the delay was added. If we don't get any response there within say 1 week, then I think we should just land this patch.
--
To view, visit https://review.coreboot.org/c/flashrom/+/80807?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: Ie09651fede3f9f03425244c94a2da8bae00315fc
Gerrit-Change-Number: 80807
Gerrit-PatchSet: 3
Gerrit-Owner: Brian Norris <briannorris(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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Tue, 23 Apr 2024 02:56:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Stefan Reinauer, Victor Lim.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81970?usp=email )
Change subject: <Flashrom SPI NOR>: adding a few new SPI NOR part numbers
......................................................................
Patch Set 1:
(3 comments)
Patchset:
PS1:
> Victor, this is amazing, thank you so much for adding and updating so many chips! […]
+1 Thank you Victor for adding all these chips!
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/81970/comment/90c2f7a2_428f544c :
PS1, Line 6998:
Just a small thing but please ensure that the new entries are indented using tabs only.
https://review.coreboot.org/c/flashrom/+/81970/comment/516b3ced_d414beb0 :
PS1, Line 7278: acts like SEC
Should this be `acts like TB`?
--
To view, visit https://review.coreboot.org/c/flashrom/+/81970?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: I1526eed830845d5391ca14c7fd3ac58a5aee71f2
Gerrit-Change-Number: 81970
Gerrit-PatchSet: 1
Gerrit-Owner: Victor Lim <victorswlim(a)yahoo.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: Victor Lim <victorswlim(a)yahoo.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 23 Apr 2024 01:30:45 +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: Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine.
Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80807?usp=email )
Change subject: flashrom: Don't throw around "delay 1 second" so lightly
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS1:
> > If nobody knows why this is necessary or how to reproduce, I think it's okay to remove. […]
Unfortunately, this hack survived so long it migrated from the CLI to the library. So a CLI flag won't really provide an escape hatch for all users -- unless you also want to also add a libflashrom flag for it??
I'm still somewhat of a lazy opinion here:
1. I doubt that anything will come of either this CL, nor of a CLI flag that provides a bug-report-escape-hatch for it and
2. the whole `--verify` sequence is trivial enough to emulate on one's own [1]. That gets slightly more complex to implement if people are using environments without basic stuff like a decent shell or tools like `cmp`, or if they're using surgical flags (like `--include`). But it's not that hard.
And the good news is we don't have to try #2 if I'm right about #1 😊
But if you still think we need a flag, I can give it a shot.
And either way, do you want me to start such a mailing list thread, or were you suggesting I do this?
[1] e.g., `flashrom --read /tmp/foo.bin`; `flashrom --write /my/new/image.bin --noverify`; `sleep 1`; `flashrom --read /tmp/bar.bin`; `cmp /tmp/foo.bin /tmp/bar.bin`
--
To view, visit https://review.coreboot.org/c/flashrom/+/80807?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: Ie09651fede3f9f03425244c94a2da8bae00315fc
Gerrit-Change-Number: 80807
Gerrit-PatchSet: 3
Gerrit-Owner: Brian Norris <briannorris(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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Tue, 23 Apr 2024 00:23:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Brian Norris, Nikolai Artemiev, Peter Marheine.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80807?usp=email )
Change subject: flashrom: Don't throw around "delay 1 second" so lightly
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS1:
> If nobody knows why this is necessary or how to reproduce, I think it's okay to remove.
Yes, true.
I was trying to find out which exactly chips needed that extra delay, but no success so far. If we knew which chips we could add a feature flag as Nikolai suggested, and mark them in flashchips. But it seems, we don't know.
So your plan looks very reasonable: an option will cover for the edge cases (if they exist).
I was thinking to start from a post on a mailing list, very specifically about removing 1s delay (there has been none dedicated thread), so that if someone suddenly recalls information about those odd chips, they can share. Even if no one responds, it stays in archives as a record of decision being made with explanation why.
And then update this patch with an option, post link to patch to the thread. That should be enough for the "paperwork" :)
--
To view, visit https://review.coreboot.org/c/flashrom/+/80807?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: Ie09651fede3f9f03425244c94a2da8bae00315fc
Gerrit-Change-Number: 80807
Gerrit-PatchSet: 3
Gerrit-Owner: Brian Norris <briannorris(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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 22 Apr 2024 11:03:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nikolai Artemiev, Stefan Reinauer, Victor Lim.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81970?usp=email )
Change subject: <Flashrom SPI NOR>: adding a few new SPI NOR part numbers
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Victor, this is amazing, thank you so much for adding and updating so many chips!
What you need to do now, is to split this large patch into several smaller ones. I can see some of the models are related and can be grouped together. So for example, GD25LB128E, GD25LQ128E, GD25LR128E can be in one patch.
And then GD25LB256F, GD25LQ256H, GD25LR256F can be in the next patch, and so on.
When you split your git commit locally, you will have a chain of commits in your local branch. And then when you push the whole chain to Gerrit, you will see all the patches in a chain as well.
For commit titles, you can use the pattern:
`flashchips: Add (or update) chip models <list models>`
Another thing is, you need to provide links to public datasheets for the models added or updated in each patch.
Thanks!
--
To view, visit https://review.coreboot.org/c/flashrom/+/81970?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: I1526eed830845d5391ca14c7fd3ac58a5aee71f2
Gerrit-Change-Number: 81970
Gerrit-PatchSet: 1
Gerrit-Owner: Victor Lim <victorswlim(a)yahoo.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: Victor Lim <victorswlim(a)yahoo.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Mon, 22 Apr 2024 10:42:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment