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/+/81792?usp=email )
Change subject: flashchips: Add write-protect support for MX25L12833F
......................................................................
Patch Set 2:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/81792/comment/2b48494f_9d6a72f4 :
PS2, Line 9008: …
[View More]MX25L12833F/MX25L12835F/MX25L12845E/MX25L12865E/MX25L12873F
This is multi-chip entry, and two of them MX25L12845E and MX25L12865E has no configuration register. So you can't add the same write-protect support to this entry.
What you can do, and that would be great to do, is to split this large entry into smaller ones, according to their write-protect specs.
The chip you are extending MX25L12833F has configuration register and your change would work. Potentially the same can work for MX25L12835F and MX25L12873F, but you need to check datasheets.
In any case, MX25L12845E/MX25L12865E need to be extracted into separate entry, because they have no configuration register.
Whether the other three can be grouped together or not, depends on the datasheets.
Let me know how it goes.
--
To view, visit https://review.coreboot.org/c/flashrom/+/81792?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: I379c833eea3ed3487504126f45c6df672a772ddc
Gerrit-Change-Number: 81792
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-CC: Hsuan Ting Chen <roccochen(a)chromium.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, 12 Apr 2024 11:39:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
[View Less]
Attention is currently required from: Hsuan-ting Chen, Stefan Reinauer.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81356?usp=email )
Change subject: ich: Add names for region 5, 9, 10, 11, 12, 15
......................................................................
Patch Set 5:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/81356/comment/b5df598a_c72a291b :
PS1, Line 12: * Incorporate missing region names from …
[View More]https://github.com/coreboot/coreboot/blob/main/util/ifdtool/ifdtool.c for completeness.
> I believe there should be a document with its name like 'MTL SPI Programming Guide,' but I don't thi […]
Just wanted to follow up on this, since the patch is waiting for a while. I have checked that naming aligns with coreboot code, but that's as much as I can do.
My interest to read docs goes beyond this patch, I would like to build more knowledge on ich. I heard about the infamous situation with lots of open-source code based on non-public docs :(
At the very least, is this testable by for example printing the descriptors for specific models?
File util/ich_descriptors_tool/ich_descriptors_tool.c:
https://review.coreboot.org/c/flashrom/+/81356/comment/08659379_5ecc8379 :
PS5, Line 42: 10GbE0
This one occurrence that I noticed when the new name is different from the old one.
Everywhere else "unknown" is replaced with new name, but the old existing name is not changed.
Is this intentional? Does it matter?
--
To view, visit https://review.coreboot.org/c/flashrom/+/81356?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: I3b164ce4ae84bfd523fcd8be416c5d13183ed632
Gerrit-Change-Number: 81356
Gerrit-PatchSet: 5
Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.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: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Comment-Date: Fri, 12 Apr 2024 10:15:32 +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
[View Less]
Attention is currently required from: Brian Norris, Peter Marheine, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81545?usp=email )
Change subject: udelay: only use OS time for delays, except on DOS
......................................................................
Patch Set 9:
(1 comment)
File tests/udelay.c:
https://review.coreboot.org/c/flashrom/+/81545/comment/c32557d5_cb836fec :
PS9, Line 41: This test could …
[View More]fail spuriously on a heavily-loaded system
> Carrying a patch to the test on ChromeOS seems okay to me, but I'm not particularly attached to test […]
Okay I have some more thoughts.
What would happens if later there is a change that by accident makes the loop infinite, would the test fail on such a change, without upper bound? Probably will result in timeout in tests, but that means tests take super long time to run.
In other words, I am thinking, maybe having any upper bound is better than having none at all, because it will assert the loop is not infinite. I agree to add 00 to the upper bound, better than dropping it at all.
What do you think, is this a useful thought?
--
To view, visit https://review.coreboot.org/c/flashrom/+/81545?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: I7ac5450d194a475143698d65d64d8bcd2fd25e3f
Gerrit-Change-Number: 81545
Gerrit-PatchSet: 9
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org>
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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Fri, 12 Apr 2024 09:06:43 +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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
[View Less]
Attention is currently required from: Anastasia Klimchuk, Brian Norris, Thomas Heijligen.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81545?usp=email )
Change subject: udelay: only use OS time for delays, except on DOS
......................................................................
Patch Set 9:
(2 comments)
Patchset:
PS9:
> Peter, this looks cool ! What do you think about sharing the link to the patch on the mailing list, […]
…
[View More]Yup, good idea. Added a note to the existing thread.
File tests/udelay.c:
https://review.coreboot.org/c/flashrom/+/81545/comment/2ab2c0fc_d1278800 :
PS9, Line 41: This test could fail spuriously on a heavily-loaded system
> From my side, I would want to keep the test, with both lower and upper bound. […]
Carrying a patch to the test on ChromeOS seems okay to me, but I'm not particularly attached to testing the upper bound anyway since it doesn't feel very right to me to have a test that can fail when run on a machine that's busy doing other things.
The invariant that the implementation needs to enforce is that it waits for no less time than requested; the upper bound is only important in that waiting for a longer time might result in a poor user experience. The question is whether we consider a poor user experience a true error, and if so what threshold we care about.
--
To view, visit https://review.coreboot.org/c/flashrom/+/81545?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: I7ac5450d194a475143698d65d64d8bcd2fd25e3f
Gerrit-Change-Number: 81545
Gerrit-PatchSet: 9
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org>
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 12 Apr 2024 06:28:44 +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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
[View Less]
Attention is currently required from: Brian Norris, Peter Marheine, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81545?usp=email )
Change subject: udelay: only use OS time for delays, except on DOS
......................................................................
Patch Set 9:
(2 comments)
Patchset:
PS9:
Peter, this looks cool ! What do you think about sharing the link to the patch on the mailing list, perhaps …
[View More]some people would be interested to help testing. Even building on environment which is not-like-Jenkins would be really helpful.
File tests/udelay.c:
https://review.coreboot.org/c/flashrom/+/81545/comment/2a494375_efde4c23 :
PS9, Line 41: This test could fail spuriously on a heavily-loaded system
> Since this test is mostly meant to exercise the sleep-free case (as I explained in response to Anast […]
From my side, I would want to keep the test, with both lower and upper bound. I like more tests, not less! :) I really appreciate Peter adding a test in a patch!
Jenkins seems to be happy with the test, as we see. We can get incoming patches from anyone in the world, and unit tests are easy to run, and they give more confidence in the patch (for both sides).
In any case I agree to keep an eye on how the test is doing after it's merged, if it causes many flakes for upstream development we can remove upper bound. But I don't think there will be troubles.
ChromeOS fork could perhaps make a small one line diff, change the assert line, or disable the test (but first option is better, much less intrusive for downstreaming). It's not the only diff in unit tests. Peter you are closer to that, do you think it's fine?
--
To view, visit https://review.coreboot.org/c/flashrom/+/81545?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: I7ac5450d194a475143698d65d64d8bcd2fd25e3f
Gerrit-Change-Number: 81545
Gerrit-PatchSet: 9
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org>
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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Fri, 12 Apr 2024 05:51:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
[View Less]