January 2018 - flashrom-gerrit

Change in flashrom[master]: cli_classic: Add --chip-size command
by Nico Huber (Code Review) Jan. 26, 2018

Jan. 26, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23346 ) Change subject: cli_classic: Add --chip-size command ...................................................................... Patch Set 3: Code-Review+2 (2 comments) https://review.coreboot.org/#/c/23346/3//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/23346/3//COMMIT_MSG@9 PS3, Line 9: This adds a command to print the size of the detected flash chip (in bytes). Line is too long :-P https://review.coreboot.org/#/c/23346/2/cli_classic.c File cli_classic.c: https://review.coreboot.org/#/c/23346/2/cli_classic.c@129 PS2, Line 129: 0x0101 > Change this (and the one for --ifd) to use an enum? Good idea. I also thought about making the operation a single variable (with a bitmask enum), currently *_it and chip_size are all mutually exclusive booleans. -- To view, visit https://review.coreboot.org/23346 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia6f334654bea73478f369207e6f4345bcb72e8d4 Gerrit-Change-Number: 23346 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 26 Jan 2018 18:58:16 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: flashchips: reset a M25P10-A before a probe
by Nico Huber (Code Review) Jan. 26, 2018

Jan. 26, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23337 ) Change subject: flashchips: reset a M25P10-A before a probe ...................................................................... Patch Set 2: I'm don't feel experienced enough to say anything about such flash chip quirks. David, any clue? -- To view, visit https://review.coreboot.org/23337 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I19a5332756d15b3cb74609c44a8d3fed2f4a2d68 Gerrit-Change-Number: 23337 Gerrit-PatchSet: 2 Gerrit-Owner: Lubomir Rintel <lkundrak(a)v3.sk> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 26 Jan 2018 18:37:54 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: digilent_spi: add a driver for the iCEblink40 development board
by Nico Huber (Code Review) Jan. 26, 2018

Jan. 26, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23338 ) Change subject: digilent_spi: add a driver for the iCEblink40 development board ...................................................................... Patch Set 2: (15 comments) Code looks quite nice :) haven't looked at the protocol yet, though. https://review.coreboot.org/#/c/23338/2/digilent_spi.c File digilent_spi.c: https://review.coreboot.org/#/c/23338/2/digilent_spi.c@14 PS2, Line 14: version 2 of the License is this restriction intentional? I've just noticed, most of flashrom is licensed v2+ https://review.coreboot.org/#/c/23338/2/digilent_spi.c@32 PS2, Line 32: #if CONFIG_DIGILENT_SPI == 1 This is unnecessary, the Makefile already takes care of it. https://review.coreboot.org/#/c/23338/2/digilent_spi.c@58 PS2, Line 58: 0 nit, spurious 0? https://review.coreboot.org/#/c/23338/2/digilent_spi.c@103 PS2, Line 103: 0 please always set a reasonable timeout https://review.coreboot.org/#/c/23338/2/digilent_spi.c@120 PS2, Line 120: return res_len; Rather `tx_len`? how about an error message for short replies? https://review.coreboot.org/#/c/23338/2/digilent_spi.c@184 PS2, Line 184: real_speed =(res[5] << 24) |(res[4] << 16) |(res[3] << 8) | res[2]; Missing spaces after = and | operators. https://review.coreboot.org/#/c/23338/2/digilent_spi.c@241 PS2, Line 241: static int digilent_spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) please break this line (112 chars limit for flashrom) https://review.coreboot.org/#/c/23338/2/digilent_spi.c@249 PS2, Line 249: buf = alloca(len); I don't know how portable it is (and it's not used yet in flashrom). I'd prefer malloc(). https://review.coreboot.org/#/c/23338/2/digilent_spi.c@253 PS2, Line 253: &buf[writecnt] `buf + writecnt` would be much clearer to me. But that could be just me. ;) https://review.coreboot.org/#/c/23338/2/digilent_spi.c@264 PS2, Line 264: if (ret != 0) Error message, please. https://review.coreboot.org/#/c/23338/2/digilent_spi.c@269 PS2, Line 269: if (ret != 0) Same here. https://review.coreboot.org/#/c/23338/2/digilent_spi.c@303 PS2, Line 303: What about the libusb handle? https://review.coreboot.org/#/c/23338/2/digilent_spi.c@356 PS2, Line 356: spiclock Other programmers call it `spispeed`. https://review.coreboot.org/#/c/23338/2/digilent_spi.c@362 PS2, Line 362: return -1; `goto close_handle;` ? https://review.coreboot.org/#/c/23338/2/digilent_spi.c@374 PS2, Line 374: if (reset_board) { : gpio_open(); : gpio_set_dir(1); : gpio_set_value(0); : } : : spi_open(); : spi_set_speed(speed_hz); : spi_set_mode(0x00); Check return values, please. -- To view, visit https://review.coreboot.org/23338 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ffcd9a2db4395816f0e8b6ce6c3b0d8e930c9e6 Gerrit-Change-Number: 23338 Gerrit-PatchSet: 2 Gerrit-Owner: Lubomir Rintel <lkundrak(a)v3.sk> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 26 Jan 2018 18:34:36 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: [v4, 6/6] Add support for reading the current flash contents from a file
by Nico Huber (Code Review) Jan. 26, 2018

Jan. 26, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23263 ) Change subject: [v4,6/6] Add support for reading the current flash contents from a file ...................................................................... Patch Set 1: Reading is faster in many cases now, is this still needed / wanted? -- To view, visit https://review.coreboot.org/23263 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idf153b6955f37779ae9bfb228a434ed10c304947 Gerrit-Change-Number: 23263 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 26 Jan 2018 16:35:00 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: [v4, 3/6] ENE EDI - add dummy read to ensure proper detection of ENE c...
by Nico Huber (Code Review) Jan. 26, 2018

Jan. 26, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23260 ) Change subject: [v4,3/6] ENE EDI - add dummy read to ensure proper detection of ENE chips ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/#/c/23260/1/edi.c File edi.c: https://review.coreboot.org/#/c/23260/1/edi.c@500 PS1, Line 500: edi_detect(flash); `edi_detect` sounds a little like it would already probe for it... how about `edi_prepare` or `edi_wakeup`? -- To view, visit https://review.coreboot.org/23260 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69ee71674649cd8ba4fc635f889cb39a1cd204b9 Gerrit-Change-Number: 23260 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 26 Jan 2018 16:24:04 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: [v4, 5/6] Add the hidden-from-probing chips category and make KB9012 h...
by Nico Huber (Code Review) Jan. 26, 2018

Jan. 26, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23262 ) Change subject: [v4,5/6] Add the hidden-from-probing chips category and make KB9012 hidden ...................................................................... Patch Set 1: (1 comment) This should be done before adding the EDI driver. AFAIR, Paul had something more flexible in mind: Instead of the `hidden` flag a grouping of the chips. Basically the same what you did but instead of 0/1 values a default of JEDEC and alternatively EDI and whatever might come in the future. How about that? https://review.coreboot.org/#/c/23262/1/flashrom.c File flashrom.c: https://review.coreboot.org/#/c/23262/1/flashrom.c@1213 PS1, Line 1213: if (!((!chip_to_probe && !chip->hidden) || This can be written much cleaner without multiple negations, IMO: if (!chip_to_probe && chip->hidden) continue; if (chip_to_probe && /* original condition ... */ You wouldn't need a comment then to explain what you do, because it would be obvious. But let's discuss the "hidden vs. groups" thing, first. -- To view, visit https://review.coreboot.org/23262 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a53ccaef2791a2ac32904d7ab813da7478a6f0 Gerrit-Change-Number: 23262 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Kocialkowski <contact(a)paulk.fr> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 26 Jan 2018 16:22:00 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: [v4, 4/6] ENE EDI - print debug info like others while probing for ENE...
by Nico Huber (Code Review) Jan. 26, 2018

Jan. 26, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23261 ) Change subject: [v4,4/6] ENE EDI - print debug info like others while probing for ENE chips ...................................................................... Patch Set 1: (4 comments) https://review.coreboot.org/#/c/23261/1/edi.c File edi.c: https://review.coreboot.org/#/c/23261/1/edi.c@154 PS1, Line 154: 0x%x It's a signed integer, why print it in hex? https://review.coreboot.org/#/c/23261/1/edi.c@164 PS1, Line 164: msg_cdbg("%s: rc1 0x%x, rc2 0x%x; ", __func__, rc1, rc2); No need to print them, they are 0 on this path. https://review.coreboot.org/#/c/23261/1/edi.c@169 PS1, Line 169: } else Please add braces here too (they are not required by syntax but help to maintain visual balance). https://review.coreboot.org/#/c/23261/1/edi.c@171 PS1, Line 171: hwversion, chip->hwversion, ediid, chip->ediid); Actually, I'm not sure if we want this message at all. This function is supposed to be called multiple times with different ids. So it's expected that they don't match. If you want to keep it, please use a higher debug level, e.g. msg_cdbg2(). -- To view, visit https://review.coreboot.org/23261 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8e62bc9f6785b4bf0be0aaf0f74c8120d77c0d4 Gerrit-Change-Number: 23261 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 26 Jan 2018 16:05:01 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: [v4, 1/6] Add support for selecting the erased bit value with a flag
by Nico Huber (Code Review) Jan. 26, 2018

Jan. 26, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23258 ) Change subject: [v4,1/6] Add support for selecting the erased bit value with a flag ...................................................................... Patch Set 1: (5 comments) Code looks good, how well was the rebase tested? https://review.coreboot.org/#/c/23258/1//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/23258/1//COMMIT_MSG@2 PS1, Line 2: Author: Mike Banon <mikebdp2(a)gmail.com> As a courtesy, you should reset the author to Paul, or use his commits from [1] as basis. [1] http://git.code.paulk.fr/gitweb/?p=flashrom.git;a=shortlog;h=refs/heads/next https://review.coreboot.org/#/c/23258/1//COMMIT_MSG@7 PS1, Line 7: [v4,1/6] Please remove this tag. https://review.coreboot.org/#/c/23258/1//COMMIT_MSG@17 PS1, Line 17: Signed-off-by: Mike Banon <mikebdp2(a)gmail.com> Please also keep original Signed-off-by lines. https://review.coreboot.org/#/c/23258/1/flashrom.c File flashrom.c: https://review.coreboot.org/#/c/23258/1/flashrom.c@762 PS1, Line 762: static int need_erase_gran_bytes(const uint8_t *have, const uint8_t *want, unsigned int len, unsigned int gran, const uint8_t erased_value) line is too long (112 chars limit) https://review.coreboot.org/#/c/23258/1/flashrom.c@792 PS1, Line 792: int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran, const uint8_t erased_value) line is too long -- To view, visit https://review.coreboot.org/23258 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia7b0de8568e31f9bf263ba0ad6b051e837477b6b Gerrit-Change-Number: 23258 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 26 Jan 2018 15:53:07 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: chipset_enable: Mark VX855 as tested
by build bot (Jenkins) (Code Review) Jan. 26, 2018

Jan. 26, 2018

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23158 ) Change subject: chipset_enable: Mark VX855 as tested ...................................................................... Patch Set 2: Build Successful https://qa.coreboot.org/job/flashrom-customrules/1081/ : SUCCESS -- To view, visit https://review.coreboot.org/23158 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f48b49ccb760f69d676ec6cbb233e532b12fbe8 Gerrit-Change-Number: 23158 Gerrit-PatchSet: 2 Gerrit-Owner: Lubomir Rintel <lkundrak(a)v3.sk> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 26 Jan 2018 15:27:56 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: chipset_enable: Mark VX855 as tested
by Nico Huber (Code Review) Jan. 26, 2018

Jan. 26, 2018

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/23158 ) Change subject: chipset_enable: Mark VX855 as tested ...................................................................... chipset_enable: Mark VX855 as tested I can confirm a successful reading and writing of SST49LF080A (LPC) on a Wyse Cx0 Thin Client (Phoenix BIOS 1.0G). Change-Id: I8f48b49ccb760f69d676ec6cbb233e532b12fbe8 Signed-off-by: Lubomir Rintel <lkundrak(a)v3.sk> Reviewed-on: https://review.coreboot.org/23158 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Paul Menzel <paulepanter(a)users.sourceforge.net> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M chipset_enable.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Nico Huber: Looks good to me, approved diff --git a/chipset_enable.c b/chipset_enable.c index a46651f..8243455 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -1713,7 +1713,7 @@ {0x1106, 0x8231, NT, "VIA", "VT8231", enable_flash_vt823x}, {0x1106, 0x8324, OK, "VIA", "CX700", enable_flash_vt823x}, {0x1106, 0x8353, NT, "VIA", "VX800/VX820", enable_flash_vt_vx}, - {0x1106, 0x8409, NT, "VIA", "VX855/VX875", enable_flash_vt_vx}, + {0x1106, 0x8409, OK, "VIA", "VX855/VX875", enable_flash_vt_vx}, {0x1106, 0x8410, OK, "VIA", "VX900", enable_flash_vt_vx}, {0x1166, 0x0200, OK, "Broadcom", "OSB4", enable_flash_osb4}, {0x1166, 0x0205, OK, "Broadcom", "HT-1000", enable_flash_ht1000}, -- To view, visit https://review.coreboot.org/23158 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I8f48b49ccb760f69d676ec6cbb233e532b12fbe8 Gerrit-Change-Number: 23158 Gerrit-PatchSet: 2 Gerrit-Owner: Lubomir Rintel <lkundrak(a)v3.sk> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0