October 2023 - flashrom-gerrit

[XS] Change in flashrom[main]: flashchips: Mark FM25F01 as tested for read/write/probe/erase
by Edward O'Callaghan (Code Review) 17 Oct '23

17 Oct '23

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78397?usp=email ) Change subject: flashchips: Mark FM25F01 as tested for read/write/probe/erase ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/78397?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: Ibb65a4cb3345eb67c049aa4d8bfd3260f4bf96db Gerrit-Change-Number: 78397 Gerrit-PatchSet: 1 Gerrit-Owner: 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: 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: Tue, 17 Oct 2023 06:00:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: flashchips: Mark FM25F01 as tested for read/write/probe/erase
by Anastasia Klimchuk (Code Review) 17 Oct '23

17 Oct '23

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/78397?usp=email ) Change subject: flashchips: Mark FM25F01 as tested for read/write/probe/erase ...................................................................... flashchips: Mark FM25F01 as tested for read/write/probe/erase As reported on the mailing list: https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/thread/JSCQ… Change-Id: Ibb65a4cb3345eb67c049aa4d8bfd3260f4bf96db Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M flashchips.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/78397/1 diff --git a/flashchips.c b/flashchips.c index e156ffb..9210de2 100644 --- a/flashchips.c +++ b/flashchips.c @@ -6042,7 +6042,7 @@ .page_size = 256, /* OTP: 256B total; enter 0x3A */ .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP, - .tested = TEST_UNTESTED, + .tested = TEST_OK_PREW, .probe = PROBE_SPI_RDID, .probe_timing = TIMING_ZERO, .block_erasers = { -- To view, visit https://review.coreboot.org/c/flashrom/+/78397?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: Ibb65a4cb3345eb67c049aa4d8bfd3260f4bf96db Gerrit-Change-Number: 78397 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[main]: doc: Add first version of code of conduct
by Anastasia Klimchuk (Code Review) 17 Oct '23

17 Oct '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/78203?usp=email ) Change subject: doc: Add first version of code of conduct ...................................................................... doc: Add first version of code of conduct As a starting point, copying coreboot's one in the absence of our own. coreboot's CoC exists for some time and is known to work, so it's a good starting point. We can iterate on this and make upgrades and amendments that make sense for flashrom community. Meanwhile we can share code of conduct with coreboot. We do have the same servers and infrastructure anyway. Change-Id: Icd82ba79687da3a2698d84f5cbfe824fbab0c426 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/78203 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> --- A doc/about_flashrom/code_of_conduct.rst M doc/about_flashrom/index.rst M doc/about_flashrom/team.rst 3 files changed, 144 insertions(+), 3 deletions(-) Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved diff --git a/doc/about_flashrom/code_of_conduct.rst b/doc/about_flashrom/code_of_conduct.rst new file mode 100644 index 0000000..da94e29 --- /dev/null +++ b/doc/about_flashrom/code_of_conduct.rst @@ -0,0 +1,140 @@ +=============== +Code of Conduct +=============== + +This code of conduct outlines our rules and expectations for everybody +participating in the flashrom community. + +flashrom community etiquette +============================ + +We have a friendly and productive atmosphere on our mailing lists, +development / code review tools, real time chat rooms and when we meet in +person. Our principles evolve around the following: + +* It's not the user's fault if something goes wrong. +* Attempt collaboration before conflict. +* People who intentionally insult others (users, developers, corporations, + other projects, or the flashrom project itself) will be dealt with. See + policy below. +* We are dealing with hardware with lots of undocumented pitfalls. It is quite + possible that you did everything right, but flashrom still won't work for you. + +Refrain from insulting anyone or the group they belong to. Remember that +people might be sensitive to other things than you are. + +Most of our community members are not native English speakers, thus +misunderstandings can (and do) happen. Assume that others are friendly +and may have picked less-than-stellar wording by accident as long as +you possibly can. + +Reporting Issues +================ + +If you have a grievance due to conduct in this community, we're sorry +that you have had a bad experience, and we want to hear about it so +we can resolve the situation. + +Please contact members of our arbitration team (listed below) promptly +and directly, in person (if available) or by email: They will listen +to you and react in a timely fashion. + +If you feel uncomfortable, please don't wait it out, ask for help, +so we can work on setting things right. + +For transparency there is no alias or private mailing list address for +you to reach out to, since we want to make sure that you know who will +and who won't read your message. + +However since people might be on travel or otherwise be unavailable +at times, please reach out to multiple persons at once, especially +when using email. + +The team will treat your messages confidential as far as the law permits. +For the purpose of knowing what law applies, the list provides the usual +country of residence of each team member. + +Unacceptable Behavior +===================== + +Unacceptable behaviors include: intimidating, harassing, abusive, +discriminatory, derogatory or demeaning speech or actions by any +participant in our community online, at all related events and in +one-on-one communications carried out in the context of community +business. Community event venues may be shared with members of the public; +please be respectful to all patrons of these locations. + +Examples of behaviors we do not accept in our community: + +* harmful or prejudicial verbal or written comments related to gender, + sexual orientation, race, religion, disability; +* inappropriate physical contact, and unwelcome sexual advances; +* deliberate intimidation, stalking or following; +* harassing photography or recording; +* sustained disruption of talks or other events. + +Using this code of conduct aggressively against other people in the +community might also be harassment. Be considerate when enforcing the code +of conduct and always try to listen to both sides before passing judgment. + +Consequences of Unacceptable Behavior +===================================== + +Unacceptable behavior from any community member, including sponsors and +those with decision-making authority, will not be tolerated. + +Anyone asked to stop unacceptable behavior is expected to comply +immediately. + +If a community member engages in unacceptable behavior, the community +organizers may take any action they deem appropriate, up to and including +a temporary ban or permanent expulsion from the community without warning +(and without refund in the case of a paid event). + +Community organizers can be members of the arbitration team, or organizers +of events and online communities. + +Addressing Grievances +===================== + +If you feel you have been falsely or unfairly accused of violating this +Code of Conduct, you should notify the arbitration team with a concise +description of your grievance. + +Legal action +============ + +Threatening or starting legal action against the project, sibling +projects hosted on coreboot.org infrastructure, project or infrastructure +maintainers leads to an immediate ban from coreboot.org and related +systems. + +The ban can be reconsidered, but it's the default action because the +people who pour lots of time and money into the projects aren't interested +in seeing their resources used against them. + +Scope +========== + +We expect all community participants (contributors, paid or otherwise; +sponsors; and other guests) to abide by this Code of Conduct in all +community venues, online and in-person, as well as in all one-on-one +communications pertaining to community business. + +Contact info +============ + +Our arbitration team consists of the following people + +* Stefan Reinauer <stefan.reinauer(a)coreboot.org> (USA) +* Patrick Georgi <patrick(a)coreboot.org> (Germany) +* Ronald Minnich <rminnich(a)coreboot.org> (USA) +* Martin Roth <martin(a)coreboot.org> (USA) + +License and attribution +======================= + +This Code of Conduct is distributed under +a `Creative Commons Attribution-ShareAlike +license <http://creativecommons.org/licenses/by-sa/3.0/>`_. It is based +on the `Citizen Code of Conduct <https://web.archive.org/web/20200330154000/http://citizencodeofconduct.org/>`_ diff --git a/doc/about_flashrom/index.rst b/doc/about_flashrom/index.rst index de36fc2..49c9849 100644 --- a/doc/about_flashrom/index.rst +++ b/doc/about_flashrom/index.rst @@ -5,3 +5,4 @@ :maxdepth: 1 team + code_of_conduct diff --git a/doc/about_flashrom/team.rst b/doc/about_flashrom/team.rst index 9d002bb..2c593ec 100644 --- a/doc/about_flashrom/team.rst +++ b/doc/about_flashrom/team.rst @@ -6,8 +6,8 @@ All contributors and users who have a Gerrit account can send patches, add comments to patches and vote +1..-1 on patches. -All contributors and users are expected to follow Development guidelines, -Code of Conduct and Friendliness guidelines. +All contributors and users are expected to follow Development guidelines and +:doc:`code_of_conduct`. There are two special groups in Gerrit. @@ -55,7 +55,7 @@ * From time to time show up in real-time channel(s) and/or dev meetings. -* Follow the Code of Conduct and Friendliness guidelines, be a good example for others. +* Follow the :doc:`code_of_conduct`, be a good example for others. * Bonus point: if you work in a [software] company, educate and help contributors from your company with upstream culture and dev guidelines. -- To view, visit https://review.coreboot.org/c/flashrom/+/78203?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: Icd82ba79687da3a2698d84f5cbfe824fbab0c426 Gerrit-Change-Number: 78203 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[main]: ichspi: Add support for C740 PCH
by Anastasia Klimchuk (Code Review) 17 Oct '23

17 Oct '23

Attention is currently required from: Kyösti Mälkki, Patrick Georgi, Patrick Rudolph, Patrick Rudolph. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78186?usp=email ) Change subject: ichspi: Add support for C740 PCH ...................................................................... Patch Set 4: (3 comments) Patchset: PS4: I was trying to find something public of "Intel SPI Programming Guide #619386" but I couldn't, is there anything public? Otherwise I will trust Patrick and David, they saw it with their own eyes :) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/78186/comment/310ad42a_1a79d106 : PS4, Line 1024: if (content->ISL <= 80) : return CHIPSET_C620_SERIES_LEWISBURG > It's not removed. This code path is never taken on C620 as ICCRIBA==0x34. […] I see now, thank you! Indeed it is just below. https://review.coreboot.org/c/flashrom/+/78186/comment/86552292_5a5efc33 : PS4, Line 490: to be compatible with 500 Series PCH below I really like your explanation, I understand now after reading it! Do you think you can put it into the code comment (instead of current one)? This one: > It says there are 6 masters (thus NM=6), but 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. -- 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: 4 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-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Comment-Date: Mon, 16 Oct 2023 23:56:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: ichspi: Add support for C740 PCH
by David Hendricks (Code Review) 16 Oct '23

16 Oct '23

Attention is currently required from: Anastasia Klimchuk, Kyösti Mälkki, Patrick Georgi, Patrick Rudolph. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78186?usp=email ) Change subject: ichspi: Add support for C740 PCH ...................................................................... Patch Set 4: Code-Review+1 -- 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: 4 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-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Comment-Date: Mon, 16 Oct 2023 17:27:46 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: flashchips: Add GD25Q128E name to the GD25Q127C/GD25Q128C entry
by Anastasia Klimchuk (Code Review) 16 Oct '23

16 Oct '23

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 1: (6 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/78348/comment/f444fee0_e6ad741c : PS1, Line 9: Q128E and Q127C/Q128C have compatible main functions, their differences : are Do you have a link to datasheet? Thank you! https://review.coreboot.org/c/flashrom/+/78348/comment/eb923048_22d6ba49 : PS1, Line 31: flashrom_tester with flashrom binary could pass with Q128E Which specifically flashrom commands did that run? Probe/read/write/erase/write-protect ? Patchset: PS1: Thanks for the patch! I have some comments, see below. File flashchips.c: https://review.coreboot.org/c/flashrom/+/78348/comment/798c0ed7_41e413e3 : PS1, Line 6802: /* FIXME: 128E's OTP: 3072B total and doesn't support QPI */ QPI part of this might be an issue. The chip definition says FEATURE_QPI is supported, so flashrom thinks it is (comments are for humans only). I don't think FEATURE_QPI is actually handled, but it doesn't seem right to say it is supported for all 3 chips. Maybe you need to split. You can copy existing definition, and put new one in alphabetical order. This will also fix the OTP comments diffs. https://review.coreboot.org/c/flashrom/+/78348/comment/e14ce1cc_585aec37 : 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, and have you tested write-protect operations? (current chip definition marks write-protect as tested). If you can check in datasheet that reg_bits are the same, but you haven't tested them, in the new definition keep reg_bits but don't mark write-protect as tested. If you don't know what are reg_bits from datasheets, don't add them (or you can add them later). Testing info is stored in `tested` field and it is an enum. File include/flashchips.h: https://review.coreboot.org/c/flashrom/+/78348/comment/dbed8e0c_205893a7 : PS1, Line 393: Same as GD25Q128B, GD25B128B, GD25Q127C, and GD25Q128E, can be distinguished by SFDP With this patch, the same model id GIGADEVICE_GD25Q128 will be used for 5 different chips, used to be 4 now 5. The macro name is not equal to any of those 5 models, but I see this is a pattern for Gigadevice :\ This patch modifies GD25Q127C, GD25Q128C, GD25Q128E, and previously there was one more entry for GD25B128B, GD25Q128B From the macro name, seems like GD25Q128B was the first one introduced. So we should say "Same as GD25B128B, GD25Q127C, GD25Q128C, GD25Q128E <and the rest of comment>" -- 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: 1 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: Mon, 16 Oct 2023 11:12:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: raiden: Support target index with generic REQ_ENABLE
by Anastasia Klimchuk (Code Review) 16 Oct '23

16 Oct '23

Attention is currently required from: Edward O'Callaghan, Jes Klinke. 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 2: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/77999/comment/2e179ba2_cc3e250a : PS2, Line 45: CL Just noticed: you need to say "this patch" "CL" is internal corp terminology, not applicable for upstream File raiden_debug_spi.c: https://review.coreboot.org/c/flashrom/+/77999/comment/0d981909_ab5554af : PS2, Line 1596: request_enable & 0xFF > See below. Yes, the EC and AP values fit inside 8 bits. […] If this is a no-op, and given your idea to change the signature of `get_target` , maybe this is not needed and third argument can stay `request_enable` ? https://review.coreboot.org/c/flashrom/+/77999/comment/80956f4e_cbbf7760 : PS2, Line 1596: request_enable & 0xFF, : (request_enable >> 8) & 0xFF, Jes, thank you for detailed explanation, I understand much better now! > Perhaps I should instead change the signature into void get_target(const struct programmer_cfg *cfg, uint8_t *request_enable, uint16_t *request_parameter) such that the function can "return" two separate values. I think this is a very good idea, and with it `request_enable` can keep the same meaning. > I have extended the Raiden protocol such that the lower byte of wValue can indicate which one of such an array of similar SPI ports to use I just thought, there is a piece of documentation at the beginning of this file, does it all still apply? I looked through quickly, seems like it doesn't need to be changed with this patch, but maybe you can check as well. Thank you! -- 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: 2 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-Comment-Date: Mon, 16 Oct 2023 09:26:30 +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

1 0

[M] Change in flashrom[main]: ichspi: Add support for C740 PCH
by Patrick Rudolph (Code Review) 16 Oct '23

16 Oct '23

Attention is currently required from: Anastasia Klimchuk, David Hendricks, Kyösti Mälkki, Patrick Georgi, Patrick Rudolph. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78186?usp=email ) Change subject: ichspi: Add support for C740 PCH ...................................................................... Patch Set 4: (3 comments) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/78186/comment/fee8885f_f805e7fe : PS4, Line 1024: if (content->ISL <= 80) : return CHIPSET_C620_SERIES_LEWISBURG > I am a bit confused, what happens with support for LEWISBURG ? Why are you just removing it? […] It's not removed. This code path is never taken on C620 as ICCRIBA==0x34. C620 is still being detected in the code path below. https://review.coreboot.org/c/flashrom/+/78186/comment/23abf067_6866f2a2 : PS4, Line 490: to be compatible with 500 Series PCH below > Maybe "to align with the SPI programming guide, which also conveniently makes it compatible with 500 […] It says there are 6 masters (thus NM=6), but 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 Intel probably only forget to update this field when writing the document. So it's not aligning with the SPI programming guide. It's more like fixing it? https://review.coreboot.org/c/flashrom/+/78186/comment/f39d3c5f_d857ef98 : PS4, Line 1039: return CHIPSET_C740_SERIES_EMMITSBURG; > For Lewisburg the ISL value should be 0x8b (139) (see also: CB:57581). […] This code path is never taken on C620 as ICCRIBA==0x34 -- 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: 4 Gerrit-Owner: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Comment-Date: Mon, 16 Oct 2023 06:34:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: ichspi: Add support for C740 PCH
by Anastasia Klimchuk (Code Review) 16 Oct '23

16 Oct '23

Attention is currently required from: Kyösti Mälkki, Patrick Georgi, Patrick Rudolph. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78186?usp=email ) Change subject: ichspi: Add support for C740 PCH ...................................................................... Patch Set 4: (3 comments) Patchset: PS4: Thanks for the patch! Jenkins fail looks temporary, should go away next time you rebase. File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/78186/comment/ab26b585_493fe6e8 : PS4, Line 1024: if (content->ISL <= 80) : return CHIPSET_C620_SERIES_LEWISBURG I am a bit confused, what happens with support for LEWISBURG ? Why are you just removing it? I remember there were lots of debates two years ago, I tried to follow the chain of patches but there are lots of abandoned ones. I can understand this line might still need a fix. But it would be in a separate commit anyway? And if it's possible to fix, would be great to fix, instead of removing ? https://review.coreboot.org/c/flashrom/+/78186/comment/5a27e98e_22bc5d8b : PS4, Line 490: to be compatible with 500 Series PCH below Maybe "to align with the SPI programming guide, which also conveniently makes it compatible with 500 Series PCH below" ? If SPI guide mentions 6 masters, you would keep it at 6 ? -- 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: 4 Gerrit-Owner: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Comment-Date: Mon, 16 Oct 2023 02:57:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: ichspi: Add support for C740 PCH
by David Hendricks (Code Review) 16 Oct '23

16 Oct '23

Attention is currently required from: Anastasia Klimchuk, Kyösti Mälkki, Patrick Georgi, Patrick Rudolph. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78186?usp=email ) Change subject: ichspi: Add support for C740 PCH ...................................................................... Patch Set 4: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/78186/comment/49ca7976_35ea16fb : PS4, Line 1039: return CHIPSET_C740_SERIES_EMMITSBURG; For Lewisburg the ISL value should be 0x8b (139) (see also: CB:57581). That matches the value seen when running `ifdtool -d` on the latest (2023.3 IPU) BKC images as well as the PCH strap length value mentioned in the C620 SPI PG (doc #559021). Not sure if it's better to fix it here or in another patch. Also, there was some discussion a while ago as to whether we should use `==` or `<=` for these comparisons, and IIRC the consensus at the time was to use '<=`. Anyway, the change you've made here for EBG looks fine, but we should probably correct the LBG value rather than removing it. -- 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: 4 Gerrit-Owner: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Comment-Date: Mon, 16 Oct 2023 00:08:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2023