August 2018 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: usbdev: Refactor device discovery code
by Daniel Thompson (Code Review) 24 Aug '18

24 Aug '18

Hello Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/27444 to look at the new patch set (#4). Change subject: usbdev: Refactor device discovery code ...................................................................... usbdev: Refactor device discovery code Currently there is a lot of code shared between usb_dev_get_by_vid_pid_serial() and usb_dev_get_by_vid_pid_number(). Fix this by pulling out the conditional filtering at the heart of each loop and calling it via a function pointer. I haven't got (two) dediprog programmers to test with but I have tested both by...serial() and by...number() calls using a pair of Developerboxen and a hacked driver. Change-Id: I31ed572501e4314b9455e1b70a5e934ec96408b1 Signed-off-by: Daniel Thompson <daniel.thompson(a)linaro.org> --- M usbdev.c 1 file changed, 70 insertions(+), 58 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/27444/4 -- To view, visit https://review.coreboot.org/27444 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: I31ed572501e4314b9455e1b70a5e934ec96408b1 Gerrit-Change-Number: 27444 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> 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]: usbdev: Refactor device discovery code
by Daniel Thompson (Code Review) 24 Aug '18

24 Aug '18

Hello Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/27444 to look at the new patch set (#3). Change subject: usbdev: Refactor device discovery code ...................................................................... usbdev: Refactor device discovery code Currently there is a lot of code shared between usb_dev_get_by_vid_pid_serial() and usb_dev_get_by_vid_pid_number(). Fix this by pulling out the conditional filtering at the heart of each loop and calling it via a function pointer. I haven't got (two) dediprog programmers to test with but I have tested both by...serial() and by...number() calls using a pair of Developerboxen and a hacked driver. Change-Id: I31ed572501e4314b9455e1b70a5e934ec96408b1 Signed-off-by: Daniel Thompson <daniel.thompson(a)linaro.org> --- M usbdev.c 1 file changed, 70 insertions(+), 58 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/27444/3 -- To view, visit https://review.coreboot.org/27444 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: I31ed572501e4314b9455e1b70a5e934ec96408b1 Gerrit-Change-Number: 27444 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> 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]: usbdev: Extract libusb1 device discovery into a separate file
by Daniel Thompson (Code Review) 24 Aug '18

24 Aug '18

Hello Paul Menzel, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/27443 to look at the new patch set (#3). Change subject: usbdev: Extract libusb1 device discovery into a separate file ...................................................................... usbdev: Extract libusb1 device discovery into a separate file Currently there is a TODO-like comment in the dediprog driver: "Might be useful for other USB devices as well". Act on this comment by collecting all the device discovery code for libusb1 devices into a separate file. Change-Id: Idfcc79371241c2c1dea97faf5e532aa971546a79 Signed-off-by: Daniel Thompson <daniel.thompson(a)linaro.org> --- M Makefile M dediprog.c M developerbox_spi.c M programmer.h A usbdev.c 5 files changed, 141 insertions(+), 108 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/27443/3 -- To view, visit https://review.coreboot.org/27443 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: Idfcc79371241c2c1dea97faf5e532aa971546a79 Gerrit-Change-Number: 27443 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de>

1 0

Change in flashrom[master]: usbdev: Refactor device discovery code
by Daniel Thompson (Code Review) 24 Aug '18

24 Aug '18

Daniel Thompson has posted comments on this change. ( https://review.coreboot.org/27444 ) Change subject: usbdev: Refactor device discovery code ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/#/c/27444/2/usbdev.c File usbdev.c: https://review.coreboot.org/#/c/27444/2/usbdev.c@84 PS2, Line 84: void * > Ack To follow up, the compiler won't let us do this. We no longer match the filter_func prototype if we change this... and other filters *do* need to alter the context pointer so we cannot change filter_t/filter_func. -- To view, visit https://review.coreboot.org/27444 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: I31ed572501e4314b9455e1b70a5e934ec96408b1 Gerrit-Change-Number: 27444 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> 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, 24 Aug 2018 15:38:26 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: usbdev: Refactor device discovery code
by Daniel Thompson (Code Review) 24 Aug '18

24 Aug '18

Daniel Thompson has posted comments on this change. ( https://review.coreboot.org/27444 ) Change subject: usbdev: Refactor device discovery code ...................................................................... Patch Set 2: (5 comments) Thanks for the review. https://review.coreboot.org/#/c/27444/2/usbdev.c File usbdev.c: https://review.coreboot.org/#/c/27444/2/usbdev.c@25 PS2, Line 25: _t > _t is for standard libraries, _func maybe? Actually, as it's only […] It is only used once but the prototype of get_by_vid_pid_filter() becomes tough to read without the typedef. I'll switch this to _func. https://review.coreboot.org/#/c/27444/2/usbdev.c@84 PS2, Line 84: void * > `const void *` to make that clear (unless the compiler won't let us) Ack https://review.coreboot.org/#/c/27444/2/usbdev.c@105 PS2, Line 105: /* no need to NULL check handle; the conditional decrement makes it unnecessary */ > No idea what this is supposed to mean. If you need to read the code […] Fair point. I'll also use the typedef to describe better how the filter works (specifically the way we call it with handle == NULL as a pre-check and handle set as a post-check). As it happens this comments is saying that if handle is non-null then it is guaranteed that *nump is false. However its probably clearer just to add a check of the handle than to explain why no check is needed. https://review.coreboot.org/#/c/27444/2/usbdev.c@118 PS2, Line 118: > no space after closing parenthesis Ack https://review.coreboot.org/#/c/27444/2/usbdev.c@115 PS2, Line 115: struct libusb_device_handle *usb_dev_get_by_vid_pid_serial( : struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, const char *serialno) : { : return get_by_vid_pid_filter(usb_ctx, vid, pid, filter_by_serial, (void *) serialno); : } > please group with the used filter function Ack -- To view, visit https://review.coreboot.org/27444 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: I31ed572501e4314b9455e1b70a5e934ec96408b1 Gerrit-Change-Number: 27444 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> 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, 24 Aug 2018 15:22:20 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add initial support for Dediprog SF200.
by David Hendricks (Code Review) 22 Aug '18

22 Aug '18

David Hendricks has uploaded this change for review. ( https://review.coreboot.org/28272 Change subject: Add initial support for Dediprog SF200. ...................................................................... Add initial support for Dediprog SF200. Change-Id: I025d1533e249f6a75b6d9015a18a6abf350456b6 Signed-off-by: Jay Thompson <thompson.jay.thomas(a)gmail.com> --- M dediprog.c 1 file changed, 12 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/72/28272/1 diff --git a/dediprog.c b/dediprog.c index 7fcadfb..933b15e 100644 --- a/dediprog.c +++ b/dediprog.c @@ -51,6 +51,7 @@ enum dediprog_devtype { DEV_UNKNOWN = 0, DEV_SF100 = 100, + DEV_SF200 = 200, DEV_SF600 = 600, }; @@ -152,7 +153,7 @@ }; const struct dev_entry devs_dediprog[] = { - {0x0483, 0xDADA, OK, "Dediprog", "SF100/SF600"}, + {0x0483, 0xDADA, OK, "Dediprog", "SF100/SF200/SF600"}, {0}, }; @@ -185,6 +186,11 @@ return PROTOCOL_V1; else return PROTOCOL_V2; + case DEV_SF200: + if (dediprog_firmwareversion < FIRMWARE_VERSION(5, 5, 0)) + return PROTOCOL_V1; + else + return PROTOCOL_V2; case DEV_SF600: if (dediprog_firmwareversion < FIRMWARE_VERSION(6, 9, 0)) return PROTOCOL_V1; @@ -800,10 +806,12 @@ msg_pdbg("Found a %s\n", buf); if (memcmp(buf, "SF100", 0x5) == 0) dediprog_devicetype = DEV_SF100; + else if (memcmp(buf, "SF200", 0x5) == 0) + dediprog_devicetype = DEV_SF200; else if (memcmp(buf, "SF600", 0x5) == 0) dediprog_devicetype = DEV_SF600; else { - msg_perr("Device not a SF100 or SF600!\n"); + msg_perr("Device not a SF100, SF200, or SF600!\n"); return 1; } @@ -1139,6 +1147,8 @@ dediprog_in_endpoint = 2; if (dediprog_devicetype == DEV_SF100) dediprog_out_endpoint = 2; + else if (dediprog_devicetype == DEV_SF200) + dediprog_out_endpoint = 2; else dediprog_out_endpoint = 1; -- To view, visit https://review.coreboot.org/28272 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I025d1533e249f6a75b6d9015a18a6abf350456b6 Gerrit-Change-Number: 28272 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>

1 0

Change in flashrom[master]: chipset_enable.c: Mark Intel HM55 as DEP
by Nico Huber (Code Review) 22 Aug '18

22 Aug '18

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/28253 ) Change subject: chipset_enable.c: Mark Intel HM55 as DEP ...................................................................... chipset_enable.c: Mark Intel HM55 as DEP Tested reading, writing and erasing the internal flash chip using an HP 630 laptop with an Intel HM55. However, since all ME-enabled chipsets are marked as DEP instead of OK, this one shall follow suit as well. Change-Id: Iaedd5bdc34dfff9b8588a3f4e1ad46460077fdf9 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-on: https://review.coreboot.org/28253 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 Nico Huber: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve diff --git a/chipset_enable.c b/chipset_enable.c index 42f6728..566b1fb 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -1805,7 +1805,7 @@ {0x8086, 0x3b06, DEP, "Intel", "H55", enable_flash_pch5}, {0x8086, 0x3b07, DEP, "Intel", "QM57", enable_flash_pch5}, {0x8086, 0x3b08, NT, "Intel", "H57", enable_flash_pch5}, - {0x8086, 0x3b09, NT, "Intel", "HM55", enable_flash_pch5}, + {0x8086, 0x3b09, DEP, "Intel", "HM55", enable_flash_pch5}, {0x8086, 0x3b0a, NT, "Intel", "Q57", enable_flash_pch5}, {0x8086, 0x3b0b, NT, "Intel", "HM57", enable_flash_pch5}, {0x8086, 0x3b0d, NT, "Intel", "3400 Mobile SFF", enable_flash_pch5}, -- To view, visit https://review.coreboot.org/28253 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: Iaedd5bdc34dfff9b8588a3f4e1ad46460077fdf9 Gerrit-Change-Number: 28253 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(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>

1 0

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

22 Aug '18

Werner Zeh 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: (1 comment) https://review.coreboot.org/#/c/23203/20//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/23203/20//COMMIT_MSG@24 PS20, Line 24: Currently it is made mutually exclusive with --ifd while still : allowing --layout until we are more clever about this input. That is not true anymore, --layout is not allowed with fmap in your latest patcheset -- 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: Wed, 22 Aug 2018 09:06:46 +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 Werner Zeh (Code Review) 22 Aug '18

22 Aug '18

Werner Zeh 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: (5 comments) That's gonna be a nice feature. Thanks for doing it. https://review.coreboot.org/#/c/23203/20/cli_classic.c File cli_classic.c: https://review.coreboot.org/#/c/23203/20/cli_classic.c@102 PS20, Line 102: 1 This should be -1! https://review.coreboot.org/#/c/23203/20/flashrom.8.tmpl File flashrom.8.tmpl: https://review.coreboot.org/#/c/23203/20/flashrom.8.tmpl@203 PS20, Line 203: Maybe add "flashrom" here? or at least an "it" would make the sentence correct. https://review.coreboot.org/#/c/23203/20/fmap.c File fmap.c: https://review.coreboot.org/#/c/23203/20/fmap.c@59 PS20, Line 59: flash What you actually check here is not the overall flashsize itself but the size of this FMAP instance in the flash. If you want you can be more precise in the comment but it's up to you. https://review.coreboot.org/#/c/23203/20/fmap.c@182 PS20, Line 182: if (!flashctx || !flashctx->chip) : return 1; Would you like to add this check to fmap_lsearch_rom() as well? https://review.coreboot.org/#/c/23203/20/fmap.c@256 PS20, Line 256: struct fmap *tmp = fmap; : fmap = realloc(fmap, fmap_len); : if (!fmap) { : msg_gerr("Failed to realloc.\n"); : free(tmp); : goto _free_ret; : } realloc is allowed to move the re-allocated memory buffer to a completely new address[1]. If that happens you will end up not freeing allocated memory. You can check if after realloc() fmap and tmp do not match and free tmp in this case. Or do I miss something here? [1] https://stackoverflow.com/questions/35413891/does-realloc-change-pointer-ad… -- 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: Wed, 22 Aug 2018 05:30:51 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

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

21 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 5: (2 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: > Ack, I checked their code... not that they cared to document […] I will add a comment to the documentation, thanks for the pointer ;) 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: ) { > same here... Necessary since an empty string will be parsed to '0'. Maybe this should be fixed in libjaylink, not sure. -- 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: 5 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: Tue, 21 Aug 2018 20:09:37 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit August 2018