January 2022 - flashrom-gerrit

Change in flashrom[master]: flashrom.c: Move {read,write}_buf_from_include_args() to layout
by Nico Huber (Code Review) 24 Jan '22

24 Jan '22

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61286 ) Change subject: flashrom.c: Move {read,write}_buf_from_include_args() to layout ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > These two functions are best situated within cli_common once read_flash_to_file() has been eliminate […] Actually, they are dead code ATM, so I find it hard to reason about intermediate steps. If I'd write this (per-region file arguments) from scratch, I'd probably go either one of these two ways: 1. Keep everything file related in CLI code. CLI would have to maintain its own mapping from region names to file names (to implement the two functions with the libflashrom API, we'd need a public API to walk the layout entries). 2. Add at set of optional, file-aware libflashrom APIs, e.g. libflashrom_file.[hc]. Something that let's the API user add file names to layout entries and then the two functions that read/write everything from/to files. -- To view, visit https://review.coreboot.org/c/flashrom/+/61286 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I61d4460ea26e07b67c99baeae5cb3840c25cb913 Gerrit-Change-Number: 61286 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 24 Jan 2022 13:04:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: fix build on non-x86 targets
by Angel Pons (Code Review) 24 Jan '22

24 Jan '22

Attention is currently required from: Nico Huber, Thomas Heijligen, Paul Menzel, Edward O'Callaghan, Peter Marheine. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61194 ) Change subject: hwaccess: fix build on non-x86 targets ...................................................................... Patch Set 7: Code-Review+1 (1 comment) File hwaccess_x86_io.c: https://review.coreboot.org/c/flashrom/+/61194/comment/a62e0fb1_4db42191 PS7, Line 82: /* PCI port I/O support is unimplemented on PPC/MIPS and unavailable on ARM. */ > Where did this go? Ah: The comment on the stub implementation of rget_io_perms() is also modified to remove references to non-x86 platforms, since that file is only built on x86 now. -- To view, visit https://review.coreboot.org/c/flashrom/+/61194 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e Gerrit-Change-Number: 61194 Gerrit-PatchSet: 7 Gerrit-Owner: Peter Marheine <pmarheine(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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 24 Jan 2022 12:43:00 +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]: hwaccess: fix build on non-x86 targets
by Angel Pons (Code Review) 24 Jan '22

24 Jan '22

Attention is currently required from: Nico Huber, Thomas Heijligen, Paul Menzel, Edward O'Callaghan, Peter Marheine. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61194 ) Change subject: hwaccess: fix build on non-x86 targets ...................................................................... Patch Set 7: (1 comment) File hwaccess_x86_io.c: https://review.coreboot.org/c/flashrom/+/61194/comment/8b25fe85_756c6d6d PS7, Line 82: /* PCI port I/O support is unimplemented on PPC/MIPS and unavailable on ARM. */ Where did this go? -- To view, visit https://review.coreboot.org/c/flashrom/+/61194 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e Gerrit-Change-Number: 61194 Gerrit-PatchSet: 7 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 24 Jan 2022 12:38:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC][WIP] Makefile: use directory for object files
by Angel Pons (Code Review) 24 Jan '22

24 Jan '22

Attention is currently required from: Nico Huber, Thomas Heijligen. Angel Pons 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 3: Code-Review+1 -- 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: 3 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Mon, 24 Jan 2022 11:45:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add comprehensive comment for chip.c
by Angel Pons (Code Review) 24 Jan '22

24 Jan '22

Attention is currently required from: Nico Huber, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59238 ) Change subject: tests: Add comprehensive comment for chip.c ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/59238 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie498ec55cce8460fc0b2e1fe27254d3a9f763fac Gerrit-Change-Number: 59238 Gerrit-PatchSet: 6 Gerrit-Owner: 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 24 Jan 2022 11:21:55 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add mediatek_i2c_spi interface.
by Angel Pons (Code Review) 24 Jan '22

24 Jan '22

Attention is currently required from: Edward O'Callaghan, Neill Corlett. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61288 ) Change subject: Add mediatek_i2c_spi interface. ...................................................................... Patch Set 1: (18 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61288/comment/a3148a58_b1d0d2b2 PS1, Line 7: . nit: no period at the end of commit summaries File mediatek_i2c_spi.c: https://review.coreboot.org/c/flashrom/+/61288/comment/91632cdc_d79530a7 PS1, Line 29: #define ISP_PORT 0x92 >> 1 : #define DEBUG_PORT 0xb2 >> 1 Please wrap these in parentheses to avoid operation order problems: #define ISP_PORT (0x92 >> 1) #define DEBUG_PORT (0xb2 >> 1) https://review.coreboot.org/c/flashrom/+/61288/comment/d5daf8c5_22b52d58 PS1, Line 36: nit: double blank line https://review.coreboot.org/c/flashrom/+/61288/comment/d4f34959_79672189 PS1, Line 55: uint8_t* buf The `*` unary operator should have a space before, not after: uint8_t *buf Same applies to the rest of the file. https://review.coreboot.org/c/flashrom/+/61288/comment/0bd79ba2_e26992b6 PS1, Line 150: len We know `len` is non-zero, special case is handled above. But I'd still keep this check here for clarity. https://review.coreboot.org/c/flashrom/+/61288/comment/661ab922_d83e71a9 PS1, Line 181: 2 ARRAY_SIZE(data) Macro is defined in flash.h https://review.coreboot.org/c/flashrom/+/61288/comment/398b455c_0c633cdd PS1, Line 187: size_t len = 1; Variable seems to be unnecessary https://review.coreboot.org/c/flashrom/+/61288/comment/8746b7f5_189ef02d PS1, Line 211: 3 ARRAY_SIZE(data) https://review.coreboot.org/c/flashrom/+/61288/comment/8cb891a9_2aa73584 PS1, Line 242: GPIO line > Is this board depended? Should this perhaps become a programmer parameter if so? i.e. […] +1 https://review.coreboot.org/c/flashrom/+/61288/comment/07329d84_73348af5 PS1, Line 249: 0x426, 7 What are these magic numbers? https://review.coreboot.org/c/flashrom/+/61288/comment/f6dbcc3e_4c2edac9 PS1, Line 255: 0x428, 7 What are these magic numbers? https://review.coreboot.org/c/flashrom/+/61288/comment/ef3219e0_8ee3973a PS1, Line 266: { Please move the brace to the following line https://review.coreboot.org/c/flashrom/+/61288/comment/283c3c0c_87002743 PS1, Line 269: unsigned char This assumes that the size of `unsigned char` is always 1 byte. I'd use `uint8_t`. https://review.coreboot.org/c/flashrom/+/61288/comment/549b8fe8_504aa10f PS1, Line 271: sizeof Along with the changes above, this should then be `ARRAY_SIZE` https://review.coreboot.org/c/flashrom/+/61288/comment/75c76fe8_12ce18ed PS1, Line 295: 0x7F Please use lowercase for hex https://review.coreboot.org/c/flashrom/+/61288/comment/32f8d483_3729467c PS1, Line 330: { Please move the brace to the following line https://review.coreboot.org/c/flashrom/+/61288/comment/3daa57d7_8c972182 PS1, Line 348: } If both attempts fail, can we assume that entering ISP has failed? https://review.coreboot.org/c/flashrom/+/61288/comment/f751156c_40623b5b PS1, Line 360: { Please move the brace to the following line -- To view, visit https://review.coreboot.org/c/flashrom/+/61288 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I24adb14e7b4f7160e1c3ff941774064d5a81e820 Gerrit-Change-Number: 61288 Gerrit-PatchSet: 1 Gerrit-Owner: Neill Corlett <corlett(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: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Neill Corlett <corlett(a)google.com> Gerrit-Comment-Date: Mon, 24 Jan 2022 11:21:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add Elkhart Lake support
by Werner Zeh (Code Review) 24 Jan '22

24 Jan '22

Attention is currently required from: Nico Huber. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60711 ) Change subject: Add Elkhart Lake support ...................................................................... Patch Set 2: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/60711/comment/961cf5ae_2cd98383 PS2, Line 15: TEST=Read and flash complete flash on Siemens MC EHL1 > Some tests with --ifd and writing only specific regions would be nice […] I have tried writing just the BIOS region with flashrom -p internal --ifd -i bios -w coreboot.rom and it went all smooth so far. File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/60711/comment/15832eed_b802c7ef PS1, Line 1033: return CHIPSET_500_SERIES_TIGER_POINT; > Please also test if it detects the right platform. Should be easy […] You nailed it, Nico. I have tried 'util/ich_descriptors_tool/ich_descriptors_tool -f coreboot.rom' with my latest mc_ehl1 coreboot image and got: Unknown flash descriptor, assuming 500 series compatibility. Assuming chipset '500 series Tiger Point'. A closer look showed that neither FLMAP1 nor FLMAP2 matches the documented settings in the SPI guide (especially the "Set to xx" comment in the register description). This is true for at least two different FIT versions I have tested as well as for the UEFI release for the Elkhart Lake CRB. I will reach out to Intel to clarify the discrepancy so that we have a chance to make it right. Will report any update here. -- 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: 2 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-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Mon, 24 Jan 2022 07:14:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add mediatek_i2c_spi interface.
by Edward O'Callaghan (Code Review) 24 Jan '22

24 Jan '22

Attention is currently required from: Angel Pons, Neill Corlett. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61288 ) Change subject: Add mediatek_i2c_spi interface. ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/61288 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I24adb14e7b4f7160e1c3ff941774064d5a81e820 Gerrit-Change-Number: 61288 Gerrit-PatchSet: 1 Gerrit-Owner: Neill Corlett <corlett(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: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Neill Corlett <corlett(a)google.com> Gerrit-Comment-Date: Mon, 24 Jan 2022 04:32:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add mediatek_i2c_spi interface.
by Edward O'Callaghan (Code Review) 24 Jan '22

24 Jan '22

Attention is currently required from: Angel Pons, Neill Corlett. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61288 ) Change subject: Add mediatek_i2c_spi interface. ...................................................................... Patch Set 1: (5 comments) Patchset: PS1: Thank you for well written the patch and contribution Neill ! Could you add a man page entry as well? I made some short comments inline File mediatek_i2c_spi.c: https://review.coreboot.org/c/flashrom/+/61288/comment/0f998c2e_f0bcce69 PS1, Line 66: d with ret value so we know how the ioctl actually failed. https://review.coreboot.org/c/flashrom/+/61288/comment/28679258_83228c63 PS1, Line 96: args.data->block args.data->block[1] ? https://review.coreboot.org/c/flashrom/+/61288/comment/40e430ca_b819fd5f PS1, Line 96: args.data->block[0] just 'len' here, avoids needing to do that indirection there to check we are in fact memcpy to a heap with enough space at all times. https://review.coreboot.org/c/flashrom/+/61288/comment/f29d3c3b_d59cbb1a PS1, Line 242: GPIO line Is this board depended? Should this perhaps become a programmer parameter if so? i.e., which gpio line to use. -- To view, visit https://review.coreboot.org/c/flashrom/+/61288 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I24adb14e7b4f7160e1c3ff941774064d5a81e820 Gerrit-Change-Number: 61288 Gerrit-PatchSet: 1 Gerrit-Owner: Neill Corlett <corlett(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: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Neill Corlett <corlett(a)google.com> Gerrit-Comment-Date: Mon, 24 Jan 2022 04:30:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Move {read,write}_buf_from_include_args() to layout
by Edward O'Callaghan (Code Review) 24 Jan '22

24 Jan '22

Attention is currently required from: Nico Huber, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61286 ) Change subject: flashrom.c: Move {read,write}_buf_from_include_args() to layout ...................................................................... Patch Set 1: Code-Review-2 (1 comment) Patchset: PS1: > Yes I asked because I noticed the rest of xxx_include_args functionality lives in layout.h. […] These two functions are best situated within cli_common once read_flash_to_file() has been eliminated from flashrom.c -- To view, visit https://review.coreboot.org/c/flashrom/+/61286 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I61d4460ea26e07b67c99baeae5cb3840c25cb913 Gerrit-Change-Number: 61286 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 24 Jan 2022 04:15:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit January 2022