February 2022 - flashrom-gerrit

Change in flashrom[master]: Add support for Adesto AT25SF128A
by Nico Huber (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Nikolai Artemiev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51097 ) Change subject: Add support for Adesto AT25SF128A ...................................................................... Patch Set 2: (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/51097/comment/e600d4c1_8e91b5e3 PS2, Line 2329: .voltage = {1700, 2000}, Adesto and Dialog both say it would be 2.7 to 3.6V. Can somebody confirm? On what boards do you have it? -- To view, visit https://review.coreboot.org/c/flashrom/+/51097 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1ce2a6699a1f0116306f668123673a1ba9c932d2 Gerrit-Change-Number: 51097 Gerrit-PatchSet: 2 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sat, 26 Feb 2022 23:29:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by David Hendricks (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Nico Huber, Martin Roth, Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 ) Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... Patch Set 9: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/cbd00c13_6d6673ec PS7, Line 24: BUG=b:215255210 > I want to make sure the reference to that bug won't do any damage. I can't […] For historical context, the "big lock" approach was intended to prevent multiples instances of flashrom from changing the target flash chip. Back then BBS (boot BIOS strap) was more of a concern, since one instance of flashrom could target a SPI ROM (e.g. BIOS) while could target the EC on LPC. Checking SCIP is a good idea, however it doesn't address the original concern that the big lock was intended for which was to prevent sudden changes to SPI controller settings during a read or write operation. See also: https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/message/HUX… https://review.coreboot.org/c/flashrom/+/61854/comment/292876ee_b6e1caff PS7, Line 25: TEST=Concurrent flashrom access is not throwing timeout. > > how since then so many years AU worked without being bothered about checking this SCIP bit in HW SEQ on older platform, I don't have that answer either with me. But for sure SW SEQ platform do use this SCIP bit checking. cros-flashrom used software sequencing for a long time because some implementations of hardware sequencing did not provide a way to write all of the status registers on recent flash chips. This meant that write protection could not be set up using hwseq. As you pointed out, software sequencing has checked the SCIP bit since commit 01d05914 when Carl-Daniel added it over a decade ago. -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 9 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Sat, 26 Feb 2022 20:35:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Nico Huber (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Martin Roth, Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 ) Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... Patch Set 9: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/a5caa685_8e004829 PS7, Line 17: Without this synchronisation being implemented, flashrom is running : into below error: : : Erasing and writing flash chip... Timeout error between offset : 0x0061c000 and 0x0061c03f (= 0x0061c000 + 63)! FAILED! : Uh oh. Erase/write failed. Checking if anything has changed. > > What I'm still missing is how it relates to the change. […] Now I'm feeling trolled. How often have I told you that the SCIP bit isn't related to sync'ing multiple processes? I've lost count. I guess after all, it's best if you'd write a commit message about SCPI only and leave the confusion out of it. Also, please don't add a reference to the bug tracker. I think that would be the best way to avoid further confusion. If you still have questions, please ask your colleagues. No need to bother independent open-source projects with Google-internal confusion. https://review.coreboot.org/c/flashrom/+/61854/comment/b866b5fa_a23fe6e1 PS7, Line 24: BUG=b:215255210 > > I don't see any answer. Please never again mark my comments as resolved. […] I want to make sure the reference to that bug won't do any damage. I can't access it so that makes it very hard for me. It seems that bug is about futility (a tool from a different project) not using the cros-flashrom big lock anymore (a concept from another project). But you reference it here as if it was associated with your change about SCIP, which it is only indirectly. Please just remove the reference. https://review.coreboot.org/c/flashrom/+/61854/comment/9a1403ef_c14e1ef7 PS7, Line 25: TEST=Concurrent flashrom access is not throwing timeout. > > > Please also test older platforms, not just the newest (also, platforms > > > that are not supported yet don't matter). If you need help with testing > > > you can always ask on the mailing list and IRC. > > > > Sorry, I don't have those older platform POC information. I will try to set a context with those owners informally. Looking at older platform EDS specification, I don't expect anything would behave differently there as this is same HW seq on Intel platform started with SKL. What's POC in this context? Owners of what? 14 year old platforms? Please just do what open-source developers do, ask for help in the community. You usually get better and quicker answers this way. > how since then so many years AU worked without being bothered about checking this SCIP bit in HW SEQ on older platform, I don't have that answer either with me. But for sure SW SEQ platform do use this SCIP bit checking. Obviously because futility moved from calling cros flashrom to using lib- flashrom lately. Why do I know that? Possibly because I'm paying attention during reviews. Please try to do so too. -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 9 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Sat, 26 Feb 2022 19:26:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Nico Huber, Martin Roth, Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 ) Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... Patch Set 9: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/84850b09_2ca1aab3 PS7, Line 25: TEST=Concurrent flashrom access is not throwing timeout. > > Please also test older platforms, not just the newest (also, platforms > > that are not supported yet don't matter). If you need help with testing > > you can always ask on the mailing list and IRC. > > Sorry, I don't have those older platform POC information. I will try to set a context with those owners informally. Looking at older platform EDS specification, I don't expect anything would behave differently there as this is same HW seq on Intel platform started with SKL. (you can always ask question about how since then so many years AU worked without this bit being implement on older platform, I don't have that answer either with me) Rephrasing how since then so many years AU worked without being bothered about checking this SCIP bit in HW SEQ on older platform, I don't have that answer either with me. But for sure SW SEQ platform do use this SCIP bit checking. -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 9 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Sat, 26 Feb 2022 17:42:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Nico Huber, Martin Roth, Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 ) Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... Patch Set 9: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61854/comment/b837a580_44b983d1 PS7, Line 17: Without this synchronisation being implemented, flashrom is running : into below error: : : Erasing and writing flash chip... Timeout error between offset : 0x0061c000 and 0x0061c03f (= 0x0061c000 + 63)! FAILED! : Uh oh. Erase/write failed. Checking if anything has changed. > What I'm still missing is how it relates to the change. isn't the observation and solution part being added in the commit msg is ample for you to understand how this changes are relevant in terms of perform sync the operation coming from different agents without having a file lock mechanism in place? > AIUI, it's > only the background story what led to its finding, and doesn't say > anything about the change. > > I think it's generally ok to add such information (even though I > don't see any value). But it must be clear how it relates to the > change. Confusing information can be harmful, especially in commit > messages that can't be changed later on. https://review.coreboot.org/c/flashrom/+/61854/comment/51d97639_71895236 PS7, Line 24: BUG=b:215255210 > I don't see any answer. Please never again mark my comments as resolved. > > Please make sure all information on that issue tracker is sane, doesn't > lead to further wrong assumptions about this change and can't be changed > anymore, e.g. close the ticket. Or, just don't reference it. Do you want me to update every post from bug b/215255210 to here? I have marked this comment resolved because I have updated the problem, observation and solution all together in this commit msg. Please be specific what you want to know from that bug. https://review.coreboot.org/c/flashrom/+/61854/comment/88cad64c_e101b92b PS7, Line 25: TEST=Concurrent flashrom access is not throwing timeout. > Please also test older platforms, not just the newest (also, platforms > that are not supported yet don't matter). If you need help with testing > you can always ask on the mailing list and IRC. Sorry, I don't have those older platform POC information. I will try to set a context with those owners informally. Looking at older platform EDS specification, I don't expect anything would behave differently there as this is same HW seq on Intel platform started with SKL. (you can always ask question about how since then so many years AU worked without this bit being implement on older platform, I don't have that answer either with me) -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 9 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Sat, 26 Feb 2022 17:24:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops
by Nico Huber (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 29: (1 comment) File ite_ecfw.c: https://review.coreboot.org/c/flashrom/+/55715/comment/ea655b2d_8a0c9373 PS29, Line 495: (uint8_t *) Ah, I was too tired yesterday when I read this and didn't see the cast. I guess this is what makes the verification work by chance? Such a cast is a no-go. -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 29 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 26 Feb 2022 16:57:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops
by Nico Huber (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 29: (2 comments) Patchset: PS29: Not sure if it was mentioned before: Generally smaller patches are preferred. Review doesn't scale well with the amount of lines and commit messages tend to be far too thin. With 10 10-times smaller patches, I generally expect 10-times faster review compared to everything at once. This one would be hard to split that much, but there are still ~4 independent topics here: board checking, flashing, content compatibilty checking (does it belong in flashrom?), the autoload option (does it belong in flashrom?). Don't know if it's too late to split. File ite_ecfw.c: https://review.coreboot.org/c/flashrom/+/55715/comment/93e4c8dd_2d3fa31d PS29, Line 411: } Doesn't this mean we always have to flash the whole chip? and all blocks in order? Should we set the write granularity to the flash size in this case? -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 29 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 26 Feb 2022 16:49:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops
by Nico Huber (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 29: (1 comment) Patchset: PS29: I'm not sure if I understand the `autoload` option correctly. Is it altering the data to be flashed? If so, how does it work with veri- fication? -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 29 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 26 Feb 2022 16:40:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops
by Nico Huber (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 29: (1 comment) Patchset: PS29: > Now I probe only for ITE Super I/Os. Just added more model IDs in the switch to catch the supported chips. Redesigning Super I/O code needs more thought and I won't pursue it right now. DMI checks have been replaced with PCI ID and SSID checks to not depend on any dmidecoder (which may fail depending on UEFI vs legacy). Generally, I'd prefer to simply hook this driver up in the internal programmer. Then it would be easiest to use the board_enable infrastructure and we wouldn't have to re- invent it. Alternatively, if you agree that the interface will change significantly in the future, we can also keep the local checks and add a FIXME comment that it actually belongs into the internal programmer. Most of the programmer drivers added lately do things wrong and I fear if we add more bad examples these issues will become worse. -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 29 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 26 Feb 2022 16:38:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops
by Nico Huber (Code Review) 26 Feb '22

26 Feb '22

Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 29: (30 comments) Patchset: PS29: Splitting an old comment thread, might make things easier to follow. > Now I probe only for ITE Super I/Os. Just added more model IDs in the switch to catch the supported chips. Redesigning Super I/O code needs more thought and I won't pursue it right now. DMI checks have been replaced with PCI ID and SSID checks to not depend on any dmidecoder (which may fail depending on UEFI vs legacy). I don't follow, can you elaborate when DMI checks would fail? File flashrom.8.tmpl: https://review.coreboot.org/c/flashrom/+/55715/comment/3b09e897_7b745a2a PS29, Line 1423: .B " flashrom \-p ite_ecfw:portpair=pairnum" Isn't this board specific? If so the information should come from board_enable or similar mechanism. https://review.coreboot.org/c/flashrom/+/55715/comment/afe36d67_5d593028 PS29, Line 1449: The default value is queried from EC. When is it needed then? https://review.coreboot.org/c/flashrom/+/55715/comment/95fb25c5_4479ffa7 PS29, Line 1471: .B " flashrom \-p ite_ecfw:boardmismatch=force" I don't think flashrom supports such a force yet nor that it should. However, for the internal programmer we allow to override the matched board. (Things would be much easier if you'd use the existing infrastructure.) File it87spi.c: https://review.coreboot.org/c/flashrom/+/55715/comment/d9b17aff_a3f5ae53 PS29, Line 105: case 0x89: How does this affect the internal programmer? Would be nice to split this out. It probably will take me hours to review. File ite_ecfw.c: https://review.coreboot.org/c/flashrom/+/55715/comment/38f9abcc_bb571d29 PS29, Line 22: #if defined(__i386__) || defined(__x86_64__) Please drop, the Makefile should decide when to compile things. https://review.coreboot.org/c/flashrom/+/55715/comment/0dc0d2c1_d161ceb4 PS29, Line 94: } __attribute__((packed)); It's one byte AFAICS, so nothing to pack. https://review.coreboot.org/c/flashrom/+/55715/comment/27c6ffd5_96ec34c6 PS29, Line 131: static int force = 0; No globals, please. https://review.coreboot.org/c/flashrom/+/55715/comment/90087253_9e3d9495 PS29, Line 133: uint8_t *buf We usually put output parameters first, like in standard C APIs. https://review.coreboot.org/c/flashrom/+/55715/comment/62d9f147_9d209ae8 PS29, Line 173: uint8_t i; You can just use an `int` or `unsigned int`. Using `uint8_t` probably just makes the compiler's job a little harder. https://review.coreboot.org/c/flashrom/+/55715/comment/607a62c3_357ef139 PS29, Line 189: } Please make sure `ec_project` gets 0 terminated, no matter what the EC replies. https://review.coreboot.org/c/flashrom/+/55715/comment/ffa1fb3e_344b434e PS29, Line 213: } Same as above. https://review.coreboot.org/c/flashrom/+/55715/comment/91c27f0e_d097cf18 PS29, Line 216: } Aside from the command these two functions look the same, any reason to repeat the code? https://review.coreboot.org/c/flashrom/+/55715/comment/c9a0a6a5_d260c45f PS29, Line 231: "You may need to pass the flash size via the programmer parameters or simply try again in a while.\n"); Please break this line around 70 chars (of text). https://review.coreboot.org/c/flashrom/+/55715/comment/2acdece3_0a341068 PS29, Line 247: /* flush the EC registers */ Is that even possible for the status register? https://review.coreboot.org/c/flashrom/+/55715/comment/a0c97201_0699b6e2 PS29, Line 268: unsigned int start, unsigned int len) Please align (I guess the intention was to, as there's already a mix of tabs and spaces). https://review.coreboot.org/c/flashrom/+/55715/comment/ba79ca23_a3417bbc PS29, Line 368: unsigned int block) Please align. https://review.coreboot.org/c/flashrom/+/55715/comment/388bde86_c750f7cf PS29, Line 392: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK Isn't this just `1*KiB`? No parentheses needed btw. https://review.coreboot.org/c/flashrom/+/55715/comment/87282e08_69db12f3 PS29, Line 394: ){ Missing space. https://review.coreboot.org/c/flashrom/+/55715/comment/46be4f43_6d6d1743 PS29, Line 413: memset(ctx_data->first_kbyte, 0, sizeof(ctx_data->first_kbyte)); What for? https://review.coreboot.org/c/flashrom/+/55715/comment/b56fceef_04e37bd1 PS29, Line 470: Spurious space. https://review.coreboot.org/c/flashrom/+/55715/comment/f73115bd_536e3e93 PS29, Line 493: ctx_data->ite_string_offset >= start && : ctx_data->ite_string_offset <= (start + len - sizeof(struct ite_string)) Shouldn't this check `start == 0` instead? How would ite_ecfw_patch_autoload() know if there is an offset? https://review.coreboot.org/c/flashrom/+/55715/comment/8aaeca21_7ff97bb3 PS29, Line 603: What about ite_ecfw_chunkwise_erase() for ITE5570? https://review.coreboot.org/c/flashrom/+/55715/comment/11b3bf6f_7bebdcb4 PS29, Line 696: } Are the IDs from RDID? I guess we should also check that the database entry assumes that. https://review.coreboot.org/c/flashrom/+/55715/comment/2ab009fe_2ea6a7ab PS29, Line 717: id_length = 4; Hmmm, if smaller chips have a smaller id, maybe it's not RDID after all. https://review.coreboot.org/c/flashrom/+/55715/comment/e884880d_506465ab PS29, Line 758: msg_perr Not an error? https://review.coreboot.org/c/flashrom/+/55715/comment/e47f5466_54b410ea PS29, Line 785: if (superio_count == 0) : return; Really not necessary, the next line checks about the same. https://review.coreboot.org/c/flashrom/+/55715/comment/fa679e10_228e3ab1 PS29, Line 812: Probing Probing might be the wrong word. AFAICS, the code just starts writing to EC RAM. https://review.coreboot.org/c/flashrom/+/55715/comment/cc2e7be5_77c4f984 PS29, Line 813: "backlight failure and sudden poweroff.\n" I assume this is copied from the text for the flash probing on legacy buses? I think it's much different as that would not write to EC registers. In this case, I guess, even severe things can happen, e.g. need to reflash the EC firmware, or even permanent damage. I don't think we want a force option for this. https://review.coreboot.org/c/flashrom/+/55715/comment/767d18c3_2a2a6a27 PS29, Line 905: int ite_ecfw_verify_file_project(uint8_t *const newcontents, Is it still call somewhere? -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 29 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 26 Feb 2022 16:34:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit February 2022