January 2022 - flashrom-gerrit

Change in flashrom[master]: spi25_statusreg: make register read/write functions generic
by Nico Huber (Code Review) 15 Jan '22

15 Jan '22

Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58475 ) Change subject: spi25_statusreg: make register read/write functions generic ...................................................................... Patch Set 19: (4 comments) File spi25_statusreg.c: https://review.coreboot.org/c/flashrom/+/58475/comment/8e41c1f8_d7b6e6da PS14, Line 51: int result = spi_send_multicommand(flash, cmds); > I think you're referring to the fact that it declares a variable in the middle of the function? Ther […] Ack File spi25_statusreg.c: https://review.coreboot.org/c/flashrom/+/58475/comment/00d231f3_77c17f09 PS15, Line 63: */ > Changed in latest patch set. Well, it's one thing to avoid a kludge for new use cases but something different to change behavior for existing use cases. Unless we want to test a lot of 15+ years old chips, please keep the behavior for SR1. I'd be ok to risk to break things, but that would have to be a commit on its own, e.g. after this series, a patch that removes the kludge even for SR1. https://review.coreboot.org/c/flashrom/+/58475/comment/c06493a7_69fdab9d PS15, Line 84: * > Done Doesn't look done. I'm not sure but I guess checkpatch would see this. Please just try. See [1] for Linus' reasoning and [2] for the style choices in coreboot (which avoids C++ comments but should be compatible with the style used here). [1] https://lkml.org/lkml/2016/7/8/625 [2] https://doc.coreboot.org/contributing/coding_style.html#commenting https://review.coreboot.org/c/flashrom/+/58475/comment/74beeeae_e612a757 PS15, Line 89: } > The next commit adds nested ifs for handling chips with different ways of writing the same register, […] Are you saying one can't use an if inside switch/case? I've looked ahead and I don't see any real problem. -- To view, visit https://review.coreboot.org/c/flashrom/+/58475 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0a3951bbf993f2d8d830143b29d3ce16cc6901d7 Gerrit-Change-Number: 58475 Gerrit-PatchSet: 19 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 15 Jan 2022 14:51:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: pcidev: Avoid internal programmer relying on pacc global
by Nico Huber (Code Review) 14 Jan '22

14 Jan '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Peter Marheine. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59275 ) Change subject: pcidev: Avoid internal programmer relying on pacc global ...................................................................... Patch Set 4: (1 comment) File pcidev.c: https://review.coreboot.org/c/flashrom/+/59275/comment/72d3158c_a727f9cc PS4, Line 154: start Doesn't it stall on the second invocation? i.e. should this be `start->next`? -- To view, visit https://review.coreboot.org/c/flashrom/+/59275 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id83bfd41f785f907e52a65a6689e8c7016fc1b77 Gerrit-Change-Number: 59275 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Fri, 14 Jan 2022 23:13:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dediprog: wait for spi bulk read xfers to finish
by Nico Huber (Code Review) 14 Jan '22

14 Jan '22

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/60087 ) Change subject: dediprog: wait for spi bulk read xfers to finish ...................................................................... dediprog: wait for spi bulk read xfers to finish dediprog_bulk_read_poll()'s finish argument allows it to be used in two distinct cases: where dediprog_bulk_read_poll will be called as part of a loop (finish=0) and where dediprog_bulk_read_poll should wait for all outstanding transfers to finish (finish=1). In both cases, dediprog_bulk_read_poll() calls libusb to process events with a 10 second timeout. After dediprog_spi_bulk_read() has queued the last transfers, it calls dediprog_bulk_read_poll() with finish=0 when it should be finish=1. finish=0 just happens to work because frequently the transfers finish in the 10 second timeout. Signed-off-by: Rick Altherr <rick(a)oxidecomputer.com> Change-Id: If7cb541742c8620358c8e04275d8316131b2d1ab Reviewed-on: https://review.coreboot.org/c/flashrom/+/60087 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M dediprog.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved diff --git a/dediprog.c b/dediprog.c index 939673c..8a05a7c 100644 --- a/dediprog.c +++ b/dediprog.c @@ -526,7 +526,7 @@ goto err_free; } /* Wait for transfers to finish. */ - if (dediprog_bulk_read_poll(dp_data->usb_ctx, &status, 0)) + if (dediprog_bulk_read_poll(dp_data->usb_ctx, &status, 1)) goto err_free; /* Check if everything has been transmitted. */ if ((status.finished_idx < count) || status.error) -- To view, visit https://review.coreboot.org/c/flashrom/+/60087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If7cb541742c8620358c8e04275d8316131b2d1ab Gerrit-Change-Number: 60087 Gerrit-PatchSet: 2 Gerrit-Owner: Rick Altherr <kc8apf(a)kc8apf.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: dediprog: wait for spi bulk read xfers to finish
by Nico Huber (Code Review) 14 Jan '22

14 Jan '22

Attention is currently required from: Rick Altherr. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60087 ) Change subject: dediprog: wait for spi bulk read xfers to finish ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: > Looks sensible to me, but I don't have a dediprog to test. It's actually a recent regression. Sneaked into a revision of CB:56414. -- To view, visit https://review.coreboot.org/c/flashrom/+/60087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If7cb541742c8620358c8e04275d8316131b2d1ab Gerrit-Change-Number: 60087 Gerrit-PatchSet: 1 Gerrit-Owner: Rick Altherr <kc8apf(a)kc8apf.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Rick Altherr <kc8apf(a)kc8apf.net> Gerrit-Comment-Date: Fri, 14 Jan 2022 15:41:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add Elkhart Lake support
by Nico Huber (Code Review) 14 Jan '22

14 Jan '22

Attention is currently required from: Werner Zeh. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60711 ) Change subject: Add Elkhart Lake support ...................................................................... Patch Set 1: Code-Review+1 (10 comments) Patchset: PS1: Nice! looks almost ready :) File chipset_enable.c: https://review.coreboot.org/c/flashrom/+/60711/comment/d7633cdc_5e8ad909 PS1, Line 709: boot_straps = boot_straps_pch500; The EDS says `bbs == 1` would be "UFS". Not sure how that's connected. Is it related to eSPI? https://review.coreboot.org/c/flashrom/+/60711/comment/8aa841ba_97c9aebe PS1, Line 995: dev So AFAICS, this is already 1f.5. We could save us the dance to find it (the PCH100 code assumes it is hidden), e.g. extract the code in enable_flash_pch100_or_c620() that works on `spi_dev` into a function that we call here directly. File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/60711/comment/acd5afa7_31fcb7bc PS1, Line 538: IFWI Intel likes to haunt us with this it seems. Now all the non-BIOS firmware is back in the TXE/ME region, but they still call it IFWI. Maybe we should just change this back to BIOS. After all, everybody is calling it the BIOS region. https://review.coreboot.org/c/flashrom/+/60711/comment/07f104ed_507f4713 PS1, Line 1033: return CHIPSET_500_SERIES_TIGER_POINT; If the SPI guide is correct and I followed it, we would end up on this path. We can identify EHL by CSSO == 0x6c (please check that in your descriptor), i.e. if (content->CSSL == 0x11) { if (content->CSSO == 0x68) return CHIPSET_500_SERIES_TIGER_POINT; if (content->CSSO == 0x6c) return CHIPSET_ELKHART_LAKE; } https://review.coreboot.org/c/flashrom/+/60711/comment/d8037f2d_edc231ea PS1, Line 1055: case CHIPSET_GEMINI_LAKE: EHL here? File ichspi.c: https://review.coreboot.org/c/flashrom/+/60711/comment/1c70e168_d2c3a495 PS1, Line 452: pprint_reg(HSFS, FLOCKDN, reg_val, "\n"); Looks like there are several new SAF (slave-attached flash?) bits. We must have missed that when adding APL already. Or actually, missed the whole function :-( I don't mind leaving them out as long as the printed bits are correct. https://review.coreboot.org/c/flashrom/+/60711/comment/bb51cd48_b6871c2e PS1, Line 464: case CHIPSET_500_SERIES_TIGER_POINT: Looks like we missed APL/GLK here? EHL should go here too it seems. https://review.coreboot.org/c/flashrom/+/60711/comment/1b8bf0b8_05ee0633 PS1, Line 1774: case CHIPSET_ELKHART_LAKE: This is tricky, the additional regs are at 0xe0. So it actually has 16. https://review.coreboot.org/c/flashrom/+/60711/comment/ccdc3fba_8b023875 PS1, Line 2027: ich_gen == CHIPSET_ELKHART_LAKE)) { Hmmm, would fit better below. Or maybe just merge the two if's. The idea was to have an accurate message, but becomes harder with every new chip. -- To view, visit https://review.coreboot.org/c/flashrom/+/60711 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I711e39a3ec9cd7098389231eaa1cb864d615a475 Gerrit-Change-Number: 60711 Gerrit-PatchSet: 1 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Comment-Date: Fri, 14 Jan 2022 14:27:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for Intel Emmitsburg PCH
by Nico Huber (Code Review) 14 Jan '22

14 Jan '22

Attention is currently required from: Jonathan Zhang. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54965 ) Change subject: Add support for Intel Emmitsburg PCH ...................................................................... Patch Set 3: (1 comment) File chipset_enable.c: https://review.coreboot.org/c/flashrom/+/54965/comment/34d05a85_5f5e98f8 PS3, Line 2076: enable_flash_c620 > Looking at the EDS (606161, rev 1.3), it seems closer to 300 series. > At least wrt. the FREG registers. Will anybody look into it? -- To view, visit https://review.coreboot.org/c/flashrom/+/54965 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2a1bb7467e693d1583aa885fa0e277075edd4a3e Gerrit-Change-Number: 54965 Gerrit-PatchSet: 3 Gerrit-Owner: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> 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: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Comment-Date: Fri, 14 Jan 2022 12:20:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Jonathan Zhang <jonzhang(a)fb.com> Comment-In-Reply-To: David Hendricks Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC][WIP] Makefile: use directory for object files
by Nico Huber (Code Review) 14 Jan '22

14 Jan '22

Attention is currently required from: Thomas Heijligen, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61034 ) Change subject: [RFC][WIP] Makefile: use directory for object files ...................................................................... Patch Set 1: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61034/comment/e83d4084_6ddf2141 PS1, Line 11: Makefile in util/ich_descriptors_tool does this already. So it hides the object files but the final binary is still in the source dir? I'm not sure if that's a good role-model. Not sure how common it is, but I'm much used to a `build/` dir for everything. File Makefile: https://review.coreboot.org/c/flashrom/+/61034/comment/7d515c12_9531c34a PS1, Line 925: _create_OBJPATH Doesn't this trigger a re-build of everything every time? https://review.coreboot.org/c/flashrom/+/61034/comment/244e328b_96b92aa6 PS1, Line 1093: -include $(OBJPATH)/$(OBJS:.o=.d) This needs `$(addprefix )`. Or maybe just do it one time, e.g. OBJS := $(addprefix $(OBJPATH)/,$(OBJS)) -- To view, visit https://review.coreboot.org/c/flashrom/+/61034 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id0b5eea0b4c941f5af37c0d27dcc95c0bb45461f Gerrit-Change-Number: 61034 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 14 Jan 2022 12:16:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Makefile: rename FEATURE_CFLAGS to FEATURE_FLAGS
by Nico Huber (Code Review) 14 Jan '22

14 Jan '22

Attention is currently required from: Thomas Heijligen. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59231 ) Change subject: Makefile: rename FEATURE_CFLAGS to FEATURE_FLAGS ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/59231 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I819f5d76e6f37a0e0ed6481a051ed85126622503 Gerrit-Change-Number: 59231 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Fri, 14 Jan 2022 11:56:58 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Makefile: merge compiler, hwlibs, features targets into config target
by Nico Huber (Code Review) 14 Jan '22

14 Jan '22

Attention is currently required from: Thomas Heijligen. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60107 ) Change subject: Makefile: merge compiler, hwlibs, features targets into config target ...................................................................... Patch Set 10: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/60107 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic76f923bca2beb6f95b8ea0cced4569b07e9b9ba Gerrit-Change-Number: 60107 Gerrit-PatchSet: 10 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Fri, 14 Jan 2022 11:55:00 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Makefile: reorder make targets
by Nico Huber (Code Review) 14 Jan '22

14 Jan '22

Attention is currently required from: Thomas Heijligen. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60836 ) Change subject: Makefile: reorder make targets ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/60836 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic45c2fc98c679ac7be4ee2860d72b517b8b67a17 Gerrit-Change-Number: 60836 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Fri, 14 Jan 2022 11:48:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit January 2022