- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: Add support for National Instruments USB-845x devices
by Miklós Márton (Code Review) 21 Aug '18

21 Aug '18

Miklós Márton has uploaded a new patch set (#9) to the change originally created by David Hendricks. ( https://review.coreboot.org/25683 ) Change subject: Add support for National Instruments USB-845x devices ...................................................................... Add support for National Instruments USB-845x devices Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Signed-off-by: Miklós Márton <martonmiklosqdev(a)gmail.com> --- M Makefile M flashrom.8.tmpl M flashrom.c A ni845x_spi.c M programmer.h 5 files changed, 640 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/25683/9 -- To view, visit https://review.coreboot.org/25683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 9 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(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-CC: Elyes HAOUAS <ehaouas(a)noos.fr> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com>

1 0

Change in flashrom[master]: Add support to get layout from fmap (e.g. coreboot rom)
by David Hendricks (Code Review) 20 Aug '18

20 Aug '18

David Hendricks has posted comments on this change. ( https://review.coreboot.org/23203 ) Change subject: Add support to get layout from fmap (e.g. coreboot rom) ...................................................................... Patch Set 20: (39 comments) Thanks for the thorough review - great suggestions as usual. I finally had some time to look through this all. Some of the proposed changes required substantial refactoring, so the code looks quite a bit different than the previous patch. It looks less like the code in cbfstool and elsewhere, but by the same token should look more like flashrom code. I re-tested --fmap with ROM images that had were bsearch-able and ones that required lsearch. --fmap-file was simplified to only use lsearch since bsearch doesn't really matter there anyway. LMK what you think. https://review.coreboot.org/#/c/23203/19/cli_classic.c File cli_classic.c: https://review.coreboot.org/#/c/23203/19/cli_classic.c@58 PS19, Line 58: fmap in <fmapfile> > s/fmap contained in ? […] Dropped the word "binary" to make it <80. https://review.coreboot.org/#/c/23203/19/cli_classic.c@619 PS19, Line 619: > NB. […] *nod* What should we call it? FLASH_SIZE_BYTES()? Or maybe flash_size_bytes() as a static inline in flash.h? https://review.coreboot.org/#/c/23203/19/fmap.h File fmap.h: https://review.coreboot.org/#/c/23203/19/fmap.h@33 PS19, Line 33: * GNU General Public License ("GPL") version 2 as published by the Free > Is the above license not GPL compatible? It's dual-licensed, as the last paragraph states. Also, in general BSD licensed code can be used in GPLv2 projects. Or maybe I'm missing something? https://review.coreboot.org/#/c/23203/19/fmap.h@44 PS19, Line 44: #define FMAP_VER_MAJOR 1 /* this header's FMAP minor version */ > If we only support v1.1 but there were lower versions, please […] Done https://review.coreboot.org/#/c/23203/19/fmap.h@55 PS19, Line 55: > writing it in big endian seems weird agreed https://review.coreboot.org/#/c/23203/19/fmap.c File fmap.c: PS19: > The mixed usage of `size_t`, `unsigned int` and `unsigned long […] OK, I went ahead and updated that in a few places. Some functions return <0 to indicate failure and >=0 if successful e.g. to indicate offset. To avoid mixing types they could be updated to take an off_t buffer as an output and return 0 or 1 to indicate failure or success. But that would make the API inconsistent with how it's used elsewhere, and probably not worth it for now. https://review.coreboot.org/#/c/23203/19/fmap.c@40 PS19, Line 40: #include "flash.h" > mmh? […] Yes. https://review.coreboot.org/#/c/23203/19/fmap.c@51 PS19, Line 51: > this is `fmap_size(fmap)`? […] Done https://review.coreboot.org/#/c/23203/19/fmap.c@57 PS19, Line 57: if (fmap->ver_major != FMAP_VER_MAJOR) > seems weird to not use a `for` loop Usually I'd agree, but I think the author may have been trying to clarify how the conditions within the loop affect the flow. I don't have a strong preference either way, so I went ahead and updated it. https://review.coreboot.org/#/c/23203/19/fmap.c@62 PS19, Line 62: > seems weird to check this inside the loop I suspect the intention here is to say that if we reach FMAP_STRLEN - 1 without quitting due to the above if-statement, then then this should return failure. Another way to write this would be: if ((i == FMAP_STRLEN - 1) && (fmap->name[i] != 0) Or perhaps these two if-statements could be combined into: if (!isgraph(fmap->name[i]) && (fmap->name[i] != 0) Anyway, even if it seems a bit awkward to you or I it might be best to leave it as is unless there is a real problem to address. https://review.coreboot.org/#/c/23203/19/fmap.c@88 PS19, Line 88: bool fmap_found = > `size_t`? Done https://review.coreboot.org/#/c/23203/19/fmap.c@89 PS19, Line 89: > `bool`? Meh. Done. https://review.coreboot.org/#/c/23203/19/fmap.c@91 PS19, Line 91: c > <= ? Yes, it should be inclusive. Done. https://review.coreboot.org/#/c/23203/19/fmap.c@91 PS19, Line 91: *)&image[offset])) { > `sizeof(struct fmap)` Yep, done. https://review.coreboot.org/#/c/23203/19/fmap.c@92 PS19, Line 92: ap at > casting to `const` is unnecessary Done https://review.coreboot.org/#/c/23203/19/fmap.c@120 PS19, Line 120: msg_gerr("Out of memory.\n"); : return > should be below the inner loop for enhanced readability Gratuitous, but will make the change anyway. https://review.coreboot.org/#/c/23203/19/fmap.c@124 PS19, Line 124: p, fmap_size(fmap)); same as above where this should be inclusive and subtract the sizeof an fmap struct https://review.coreboot.org/#/c/23203/19/fmap.c@126 PS19, Line 126: > so `&image[0]` is always checked? that doesn't seem to make much sense True, I guess it was never a problem earlier, but of course now that we might have a slow programmers it's important to not waste cycles... Anyway, I tried a couple of things and I think the simplest/cleanest solution to address that corner case is to have a variable that tracks whether we've checked offset 0. https://review.coreboot.org/#/c/23203/19/fmap.c@129 PS19, Line 129: struct flashctx *const flashctx, size_t rom_offset, size_t len) > no need to break the line Done https://review.coreboot.org/#/c/23203/19/fmap.c@158 PS19, Line 158: malloc > initialization seems never used Done. https://review.coreboot.org/#/c/23203/19/fmap.c@166 PS19, Line 166: _free_ret: > I don't want to overoptimize things, but I have the feeling that […] After thinking about this a bit, I think we should just get rid of fmap_bsearch in this file. We really only care about the bsearch if we're reading directly from ROM, but in this codepath we're only going to read from memory. https://review.coreboot.org/#/c/23203/19/libflashrom.h File libflashrom.h: https://review.coreboot.org/#/c/23203/19/libflashrom.h@64 PS19, Line 64: , > qualification has no effect here and the name shouldn't be […] OK. https://review.coreboot.org/#/c/23203/19/libflashrom.h@67 PS19, Line 67: struct flashrom_flashctx *, const uint8_t *const buf, size_t len); > The library was intentionally designed to be free from file i/o. […] Sure. It took a bit of refactoring, but done. https://review.coreboot.org/#/c/23203/19/libflashrom.c File libflashrom.c: https://review.coreboot.org/#/c/23203/19/libflashrom.c@428 PS19, Line 428: 1 o > `!*fmap_out` Done https://review.coreboot.org/#/c/23203/19/libflashrom.c@442 PS19, Line 442: msg_gdbg("Adding fmap layout to global layout.\n"); > please break this line Done https://review.coreboot.org/#/c/23203/19/libflashrom.c@466 PS19, Line 466: c > <= ? Yep, this should be inclusive. Done. https://review.coreboot.org/#/c/23203/19/libflashrom.c@466 PS19, Line 466: t flashctx *const flashctx, const uint8_t *cons > could be asserted at the beginning of the function, right? Done. I had a check in the calling function, but it's probably clearer to put it at the beginning of this function. https://review.coreboot.org/#/c/23203/19/libflashrom.c@468 PS19, Line 468: > both conditions don't make much sense, imho. it should be […] Yes - I've added a variable to track whether or not we have checked offset 0. It's a bit of a kludge, but I think it addresses this corner case in the simplest manner. This function always reads from flash so beginning at rom_offset is the most intuitive way. https://review.coreboot.org/#/c/23203/19/libflashrom.c@473 PS19, Line 473: > Since this does not bail out on the stride loop we might as well use continue instead of break? if w […] Yes, that sounds like a good idea. While we're at it, I demoted this debug print to msg_cdbg(). If there's one failure due to a locked region, there will likely be many dozens or hundreds of similar benign failures. https://review.coreboot.org/#/c/23203/19/libflashrom.c@479 PS19, Line 479: } > seems impossible by loop condition Agreed, removed. https://review.coreboot.org/#/c/23203/19/libflashrom.c@485 PS19, Line 485: goto _free_ret; > missing check for NULL Done https://review.coreboot.org/#/c/23203/19/libflashrom.c@486 PS19, Line 486: } > we could skip reading the signature again Done https://review.coreboot.org/#/c/23203/19/libflashrom.c@491 PS19, Line 491: > I'm sure I've seen a function for this. Done https://review.coreboot.org/#/c/23203/19/libflashrom.c@492 PS19, Line 492: return ret; > missing check for NULL Done https://review.coreboot.org/#/c/23203/19/libflashrom.c@494 PS19, Line 494: > we could skip reading the header again Done https://review.coreboot.org/#/c/23203/19/libflashrom.c@499 PS19, Line 499: */ > IIRC, this can be checked on the header alone Done https://review.coreboot.org/#/c/23203/19/libflashrom.c@515 PS19, Line 515: * @param layout Layout to bet set. > potentially leaking `fmap` if we didn't succeed Done https://review.coreboot.org/#/c/23203/19/libflashrom.c@442 PS19, Line 442: msg_gdbg("Adding fmap layout to global layout.\n"); : if (flashrom_layout_parse_fmap(layout, flashctx, fmap)) { : msg_gerr("Failed to add fmap regions to layout.\n"); : ret = 1; : } : : free(fmap); : return ret; : } : : /** : * @brief Read a layout by searching buffer for fmap. : * : * @param[out] layout Points to a struct flashrom_layout pointer that : * gets set if the fmap is read and parsed successfully. : * @param[in] flashctx Flash context : * @param[in] buffer Buffer to search in : * @param[in] size Size of buffer to search : * : * @return 0 on success, : * 2 if the fmap couldn't be read, : * 1 on any other error. : */ : int flashrom_layout_read_fmap_from_buffer(struct flashrom_layout **const layout, : struct flashctx *const flashctx, const uint8_t *const buf, size_t size) : { : struct fmap *fmap = NULL; : int ret = 1; : : if (!buf || !size) : goto _ret; : : msg_gdbg("Attempting to read fmap from buffer.\n"); : if (fmap_read_from_buffer(&fmap, buf, size)) { : msg_gerr("Failed to read fmap from buffer.\n"); : ret = 1; : goto _ret; : } : : msg_gdbg("Adding fmap layout to global layout.\n"); : if (flashrom_layout_parse_fmap(layout, flashctx, fmap)) { : msg_gerr("Failed to add fmap regions to layout.\n"); : ret = 1; : goto _free_ret; : } : : ret = 0; : _free_ret: : free(fmap); : _ret: : return ret; : } : : /** : * @brief Free a layout. : * : * @param layout Layout to free. : */ : void flashrom_layout_release(struct flashrom_layout *const layout) : { : if (layout == get_global_layout()) : return; : : free(layout); : } : : /** : * @brief Set the active layout for a flash context. : * : * Note: This just sets a pointer. The caller must not release the layout : * as long as he uses it through the given flash context. : * : * @param flashctx Flash context whose layout will be set. : * @param layout Layout to bet set. : This whole thing should go in fmap.c. https://review.coreboot.org/#/c/23203/19/libflashrom.c@534 PS19, Line 534: > We should either allocate it or add a check below to not overflow the […] Done -- To view, visit https://review.coreboot.org/23203 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: I0e7fad38ed79a84d41358e1f175c36d255786c12 Gerrit-Change-Number: 23203 Gerrit-PatchSet: 20 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> 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-CC: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Comment-Date: Tue, 21 Aug 2018 05:23:37 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add support to get layout from fmap (e.g. coreboot rom)
by David Hendricks (Code Review) 20 Aug '18

20 Aug '18

David Hendricks has uploaded a new patch set (#20) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/23203 ) Change subject: Add support to get layout from fmap (e.g. coreboot rom) ...................................................................... Add support to get layout from fmap (e.g. coreboot rom) Flashmap, or simply fmap, is a binary data format for describing region offsets, sizes, and certain attributes and is widely used by coreboot. This patch adds support for the fmap data format version 1.1 and adds --fmap and --fmap-file arguments. Using --fmap will make flashrom to search the ROM content for fmap data. Using --fmap-file will make flashrom search a supplied file for fmap data. An example of how to update the COREBOOT region of a ROM: flashrom -p programmer --fmap -w coreboot.rom -i COREBOOT flashrom -p programmer --fmap-file coreboot.rom -w coreboot.rom -i COREBOOT The fmap functions are mostly copied from cbfstool. Currently it is made mutually exclusive with --ifd while still allowing --layout until we are more clever about this input. Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Signed-off-by: David Hendricks <dhendricks(a)fb.com> --- M Makefile M cli_classic.c M flashrom.8.tmpl A fmap.c A fmap.h M libflashrom.c M libflashrom.h 7 files changed, 662 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/23203/20 -- To view, visit https://review.coreboot.org/23203 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12 Gerrit-Change-Number: 23203 Gerrit-PatchSet: 20 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> 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-CC: Werner Zeh <werner.zeh(a)siemens.com>

1 0

Change in flashrom[master]: Add initial J-Link SPI programmer
by Nico Huber (Code Review) 20 Aug '18

20 Aug '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/28087 ) Change subject: Add initial J-Link SPI programmer ...................................................................... Patch Set 4: (3 comments) https://review.coreboot.org/#/c/28087/2/jlink_spi.c File jlink_spi.c: https://review.coreboot.org/#/c/28087/2/jlink_spi.c@344 PS2, Line 344: > No, the device instance is not free'd because we still have a reference on it (jaylink_open()) Ack, I checked their code... not that they cared to document this side effect of jaylink_open(). https://review.coreboot.org/#/c/28087/4/jlink_spi.c File jlink_spi.c: https://review.coreboot.org/#/c/28087/4/jlink_spi.c@214 PS4, Line 214: && strlen(arg) > 0 same here... https://review.coreboot.org/#/c/28087/4/jlink_spi.c@237 PS4, Line 237: && strlen(arg) > 0 and there? -- To view, visit https://review.coreboot.org/28087 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: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e Gerrit-Change-Number: 28087 Gerrit-PatchSet: 4 Gerrit-Owner: Marc Schink <flashrom-dev(a)marcschink.de> Gerrit-Reviewer: Marc Schink <flashrom-dev(a)marcschink.de> 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: Mon, 20 Aug 2018 21:29:01 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add initial J-Link SPI programmer
by Marc Schink (Code Review) 20 Aug '18

20 Aug '18

Marc Schink has posted comments on this change. ( https://review.coreboot.org/28087 ) Change subject: Add initial J-Link SPI programmer ...................................................................... Patch Set 4: (1 comment) https://review.coreboot.org/#/c/28087/2/jlink_spi.c File jlink_spi.c: https://review.coreboot.org/#/c/28087/2/jlink_spi.c@344 PS2, Line 344: > Isn't this `dev` (as part of `devs`) free'd yet? No, the device instance is not free'd because we still have a reference on it (jaylink_open()) -- To view, visit https://review.coreboot.org/28087 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: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e Gerrit-Change-Number: 28087 Gerrit-PatchSet: 4 Gerrit-Owner: Marc Schink <flashrom-dev(a)marcschink.de> Gerrit-Reviewer: Marc Schink <flashrom-dev(a)marcschink.de> 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: Mon, 20 Aug 2018 19:16:05 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add initial J-Link SPI programmer
by Marc Schink (Code Review) 20 Aug '18

20 Aug '18

Hello Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/28087 to look at the new patch set (#4). Change subject: Add initial J-Link SPI programmer ...................................................................... Add initial J-Link SPI programmer Tested with SEGGER J-Link EDU, Flasher ARM and flash chip W25Q16.V. Change-Id: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e Signed-off-by: Marc Schink <flashrom-dev(a)marcschink.de> --- M Makefile M README M flashrom.8.tmpl M flashrom.c A jlink_spi.c M programmer.h 6 files changed, 586 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/28087/4 -- To view, visit https://review.coreboot.org/28087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e Gerrit-Change-Number: 28087 Gerrit-PatchSet: 4 Gerrit-Owner: Marc Schink <flashrom-dev(a)marcschink.de> 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

Change in flashrom[master]: Add initial J-Link SPI programmer
by Marc Schink (Code Review) 20 Aug '18

20 Aug '18

Hello Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/28087 to look at the new patch set (#3). Change subject: Add initial J-Link SPI programmer ...................................................................... Add initial J-Link SPI programmer Tested with SEGGER J-Link EDU, Flasher ARM and flash chip W25Q16.V. Change-Id: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e Signed-off-by: Marc Schink <flashrom-dev(a)marcschink.de> --- M Makefile M README M flashrom.8.tmpl M flashrom.c A jlink_spi.c M programmer.h 6 files changed, 586 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/28087/3 -- To view, visit https://review.coreboot.org/28087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e Gerrit-Change-Number: 28087 Gerrit-PatchSet: 3 Gerrit-Owner: Marc Schink <flashrom-dev(a)marcschink.de> 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

Change in flashrom[master]: Add initial J-Link SPI programmer
by Nico Huber (Code Review) 20 Aug '18

20 Aug '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/28087 ) Change subject: Add initial J-Link SPI programmer ...................................................................... Patch Set 2: (4 comments) https://review.coreboot.org/#/c/28087/1/jlink_spi.c File jlink_spi.c: https://review.coreboot.org/#/c/28087/1/jlink_spi.c@130 PS1, Line 130: > Do not free it here or set `buffer = NULL;`. Otherwise we'd […] Ping? Also, `buffer_size` should be set to zero too if we keep the free(). Btw., it would all get less complex if we'd just malloc()/free() on every call, w/o the global buffer. https://review.coreboot.org/#/c/28087/2/jlink_spi.c File jlink_spi.c: https://review.coreboot.org/#/c/28087/2/jlink_spi.c@191 PS2, Line 191: /* TODO: Use kHz for speed and update manual! */ already obsolete? https://review.coreboot.org/#/c/28087/2/jlink_spi.c@195 PS2, Line 195: && strlen(arg) > 0 Why check the strlen? if it's 0, the user literally typed `:spispeed=` without a value. Why allow that? https://review.coreboot.org/#/c/28087/2/jlink_spi.c@344 PS2, Line 344: dev Isn't this `dev` (as part of `devs`) free'd yet? -- To view, visit https://review.coreboot.org/28087 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: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e Gerrit-Change-Number: 28087 Gerrit-PatchSet: 2 Gerrit-Owner: Marc Schink <flashrom-dev(a)marcschink.de> 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: Mon, 20 Aug 2018 16:46:14 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add initial J-Link SPI programmer
by Marc Schink (Code Review) 20 Aug '18

20 Aug '18

Hello Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/28087 to look at the new patch set (#2). Change subject: Add initial J-Link SPI programmer ...................................................................... Add initial J-Link SPI programmer Tested with SEGGER J-Link EDU, Flasher ARM and flash chip W25Q16.V. Change-Id: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e Signed-off-by: Marc Schink <flashrom-dev(a)marcschink.de> --- M Makefile M README M flashrom.8.tmpl M flashrom.c A jlink_spi.c M programmer.h 6 files changed, 594 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/28087/2 -- To view, visit https://review.coreboot.org/28087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie03a054a75457ec9e1cab36ea124bb53b10e8d7e Gerrit-Change-Number: 28087 Gerrit-PatchSet: 2 Gerrit-Owner: Marc Schink <flashrom-dev(a)marcschink.de> 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

Change in flashrom[master]: Add support for National Instruments USB-845x devices
by Nico Huber (Code Review) 19 Aug '18

19 Aug '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/25683 ) Change subject: Add support for National Instruments USB-845x devices ...................................................................... Patch Set 8: (1 comment) https://review.coreboot.org/#/c/25683/8/ni845x_spi.c File ni845x_spi.c: https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@63 PS8, Line 63: /* Copied from dediprog.c */ > Actually it is copied from the pickit2 code: > https://github.com/flashrom/flashrom/blob/master/pickit2_spi.c#L302 > > I am under the impression too that these spi clock speed > calculation functions shall be uniformized & placed to a common > place. This is about voltage ;) but, yeah, `spispeed` suffers the same. > What do you recommend? Should I create an another patchset > which modifies dediprog, pickit and rebase this onto the top of > that? That would be a good way, if you want to do it. A common place for `voltage` and `spispeed` parameter parsing would be great and much appreciated. Though, be warned, when it comes to `spipeed` there are a lot more drivers to touch. -- To view, visit https://review.coreboot.org/25683 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: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 8 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(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-CC: Elyes HAOUAS <ehaouas(a)noos.fr> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com> Gerrit-Comment-Date: Sun, 19 Aug 2018 20:25:53 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit