Change in flashrom[master]: dediprog.c: Add id parameter to dediprog programmer
Ryan O'Leary has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/34160 ) Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... dediprog.c: Add id parameter to dediprog programmer When multiple dediprog programmers are connected, the 'id' parameter allows you to specify which one to use. The id is a string like SF012345 or DP012345. The value is printed on a sticker on the back of the dediprog. This is an improvement over the 'device' parameter which is based on enumeration order and changes when you plug/unplug devices or reboot the machine. To find the id without the sticker, run flashrom with the -V option. This prints the ids as they are enumerated. Alternatively, with dpcmd, you can use the --list-device-id and --fix-device commands to list and write device ids respectively. Note this only supports SF100 at the moment, but SF600 support is possible with more work. Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Signed-off-by: Ryan O'Leary <ryanoleary@google.com> --- M dediprog.c 1 file changed, 133 insertions(+), 19 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/60/34160/1 diff --git a/dediprog.c b/dediprog.c index 8029b64..1d10b24 100644 --- a/dediprog.c +++ b/dediprog.c @@ -823,6 +823,46 @@ return 0; } +/* Read the id from the dediprog. This should return the numeric part of the + * serial number found on a sticker on the back of the dediprog. Note this + * number is stored in writable eeprom, so it could get out of sync. Also note, + * this function only supports SF100 at this time, but SF600 support is not too + * much different. + * @return the id on success, -1 on failure + */ +static int dediprog_read_id(void) +{ + int ret; + uint8_t buf[3]; + + ret = libusb_control_transfer(dediprog_handle, REQTYPE_OTHER_IN, + 0x7, /* request */ + 0, /* value */ + 0xEF00, /* index */ + buf, sizeof(buf), + DEFAULT_TIMEOUT); + if (ret != sizeof(buf)) { + msg_perr("Failed to read dediprog id, error %d!\n", ret); + return -1; + } + + return buf[0] << 16 | buf[1] << 8 | buf[2]; +} + +/* Return the prefix to display in front of the id. These two characters also + * appear on the sticker even though they can be derived from the numerical + * part. + * @id a dediprog's id + * @return either "DP" or "SF" + */ +static const char* dediprog_id_prefix(int id) { + /* This is so the id matches dpcmd. */ + if (id < 600000) { + return "DP"; + } + return "SF"; +} + /* * This command presumably sets the voltage for the SF100 itself (not the * SPI flash). Only use this command with firmware older than V6.0.0. Newer @@ -997,9 +1037,11 @@ int dediprog_init(void) { - char *voltage, *device, *spispeed, *target_str; + char *voltage, *id_str, *device, *spispeed, *target_str; int spispeed_idx = 1; int millivolt = 3500; + int id = -1; /* -1 defaults to enumeration order */ + int found_id; long usedevice = 0; long target = FLASH_TYPE_APPLICATION_FLASH_1; int i, ret; @@ -1029,9 +1071,37 @@ msg_pinfo("Setting voltage to %i mV\n", millivolt); } + id_str = extract_programmer_param("id"); + if (id_str) { + char prefix0, prefix1; + const char *prefix; + if (sscanf(id_str, "%c%c%d", &prefix0, &prefix1, &id) != 3) { + msg_perr("Error: Could not parse dediprog 'id'.\n"); + msg_perr("Expected a string like SF012345 or DP012345.\n"); + free(id_str); + return 1; + } + if (id < 0 || id >= 0x1000000) { + msg_perr("Error: id %s is out of range!\n", id_str); + free(id_str); + return 1; + } + prefix = dediprog_id_prefix(id); + if (strlen(prefix) != 2 || prefix[0] != prefix0 || prefix[1] != prefix1) { + msg_perr("Error: %s is an invalid id!\n", id_str); + free(id_str); + return 1; + } + msg_pinfo("Will search for dediprog id %s%06d.\n", dediprog_id_prefix(id), id); + } + free(id_str); + device = extract_programmer_param("device"); if (device) { char *dev_suffix; + if (id != -1) { + msg_perr("Error: Cannot use 'id' and 'device'.\n"); + } errno = 0; usedevice = strtol(device, &dev_suffix, 10); if (errno != 0 || device == dev_suffix) { @@ -1097,25 +1167,69 @@ const uint16_t vid = devs_dediprog[0].vendor_id; const uint16_t pid = devs_dediprog[0].device_id; - dediprog_handle = usb_dev_get_by_vid_pid_number(usb_ctx, vid, pid, (unsigned int) usedevice); - if (!dediprog_handle) { - msg_perr("Could not find a Dediprog programmer on USB.\n"); - libusb_exit(usb_ctx); - return 1; + for (;;) { + /* Notice we can only call dediprog_read_id() after + * libusb_set_configuration() and libusb_claim_interface(). When + * searching by id and either configuration or claim fails + * (usually the device is in use by another instance of + * flashrom), the device is skipped and the next device is + * tried. + */ + dediprog_handle = usb_dev_get_by_vid_pid_number(usb_ctx, vid, pid, (unsigned int) usedevice); + if (!dediprog_handle) { + msg_perr("Could not find a Dediprog programmer on USB.\n"); + libusb_exit(usb_ctx); + return 1; + } + ret = libusb_set_configuration(dediprog_handle, 1); + if (ret != 0) { + msg_perr("Could not set USB device configuration: %i %s\n", + ret, libusb_error_name(ret)); + libusb_close(dediprog_handle); + if (id != -1) { + usedevice++; + continue; + } + libusb_exit(usb_ctx); + return 1; + } + ret = libusb_claim_interface(dediprog_handle, 0); + if (ret < 0) { + msg_perr("Could not claim USB device interface %i: %i %s\n", + 0, ret, libusb_error_name(ret)); + libusb_close(dediprog_handle); + if (id != -1) { + usedevice++; + continue; + } + libusb_exit(usb_ctx); + return 1; + } + + /* Read the dediprog's id. The value is only used (besides + * logging) when searching by id. + */ + found_id = dediprog_read_id(); + if (id != -1) { + if (found_id < 0) { + msg_perr("Could not read id.\n"); + libusb_release_interface(dediprog_handle, 0); + libusb_close(dediprog_handle); + usedevice++; + continue; + } + msg_pinfo("Found dediprog id %s%06d.\n", dediprog_id_prefix(found_id), found_id); + if (found_id != id) { + libusb_release_interface(dediprog_handle, 0); + libusb_close(dediprog_handle); + usedevice++; + continue; + } + } + break; } - ret = libusb_set_configuration(dediprog_handle, 1); - if (ret != 0) { - msg_perr("Could not set USB device configuration: %i %s\n", ret, libusb_error_name(ret)); - libusb_close(dediprog_handle); - libusb_exit(usb_ctx); - return 1; - } - ret = libusb_claim_interface(dediprog_handle, 0); - if (ret < 0) { - msg_perr("Could not claim USB device interface %i: %i %s\n", 0, ret, libusb_error_name(ret)); - libusb_close(dediprog_handle); - libusb_exit(usb_ctx); - return 1; + if (found_id >= 0) { + msg_pinfo("Using dediprog id %s%06d.\n", dediprog_id_prefix(found_id), found_id); } if (register_shutdown(dediprog_shutdown, NULL)) -- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 1 Gerrit-Owner: Ryan O'Leary Gerrit-MessageType: newchange
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34160 ) Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... Patch Set 2: Code-Review+1 (4 comments) Nice patch! Found some nits, and got one idea how the loop could be refactored. https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c File dediprog.c: https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@826 PS2, Line 826: /* Read the id from the dediprog. This should return the numeric part of the Please start comments with /* on a separate line. https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@1089 PS2, Line 1089: strlen(prefix) != 2 This is true by definition. https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@1094 PS2, Line 1094: dediprog_id_prefix(id) or just `prefix` https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@1229 PS2, Line 1229: } One thought to make this whole looping easier to read (not 100% sure if it would look better): Move the original three calls (_dev_get, _set_configuration, _claim_interface) into a separate function (e.g. dediprog_open()). Then it should be easier to separate the `id` case from the `usedevice` case: if (id != -1) { for (i = 0; ; i++) { ret = dediprog_open(i); if (ret == -1) { /* no dev */ ... return 1; } else if (ret == -2) { /* busy dev */ continue; } found_id = dediprog_read_id(); ... break; } } else { if (dediprog_open(usedevice)) { ... return 1; } found_id = dediprog_read_id(); } Or maybe even put the loop body into yet another function (makes it easier to have a common error path). -- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 2 Gerrit-Owner: Ryan O'Leary Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Ryan O'Leary Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 25 Jul 2019 22:02:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Hello ron minnich, David Hendricks, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/34160 to look at the new patch set (#3). Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... dediprog.c: Add id parameter to dediprog programmer When multiple dediprog programmers are connected, the 'id' parameter allows you to specify which one to use. The id is a string like SF012345 or DP012345. The value is printed on a sticker on the back of the dediprog. This is an improvement over the 'device' parameter which is based on enumeration order and changes when you plug/unplug devices or reboot the machine. To find the id without the sticker, run flashrom with the -V option. This prints the ids as they are enumerated. Alternatively, with dpcmd, you can use the --list-device-id and --fix-device commands to list and write device ids respectively. Note this only supports SF100 at the moment, but SF600 support is possible with more work. Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Signed-off-by: Ryan O'Leary <ryanoleary@google.com> --- M dediprog.c 1 file changed, 133 insertions(+), 21 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/60/34160/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 3 Gerrit-Owner: Ryan O'Leary Gerrit-Assignee: Ryan O'Leary Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Ryan O'Leary Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Ryan O'Leary has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34160 ) Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... Patch Set 3: (4 comments) I removed the dediprog_id_prefix function because I had a SF100 which did not follow this naming scheme. https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c File dediprog.c: https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@826 PS2, Line 826: /* Read the id from the dediprog. This should return the numeric part of the
Please start comments with /* on a separate line. Done
https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@1089 PS2, Line 1089: strlen(prefix) != 2
This is true by definition. Done
https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@1094 PS2, Line 1094: dediprog_id_prefix(id)
or just `prefix` Done
https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@1229 PS2, Line 1229: }
One thought to make this whole looping easier to read (not 100% sure if […] Done. Thanks!
-- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 3 Gerrit-Owner: Ryan O'Leary Gerrit-Assignee: Ryan O'Leary Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Ryan O'Leary Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 12 Nov 2019 19:10:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
Hello ron minnich, David Hendricks, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/34160 to look at the new patch set (#4). Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... dediprog.c: Add id parameter to dediprog programmer When multiple dediprog programmers are connected, the 'id' parameter allows you to specify which one to use. The id is a string like SF012345 or DP012345. The value is printed on a sticker on the back of the dediprog. This is an improvement over the 'device' parameter which is based on enumeration order and changes when you plug/unplug devices or reboot the machine. To find the id without the sticker, run flashrom with the -V option. This prints the ids as they are enumerated. Alternatively, with dpcmd, you can use the --list-device-id and --fix-device commands to list and write device ids respectively. Note this only supports SF100 at the moment, but SF600 support is possible with more work. Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Signed-off-by: Ryan O'Leary <ryanoleary@google.com> --- M dediprog.c 1 file changed, 134 insertions(+), 21 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/60/34160/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 4 Gerrit-Owner: Ryan O'Leary Gerrit-Assignee: Ryan O'Leary Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Ryan O'Leary Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34160 ) Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... Patch Set 4: Code-Review+1 (1 comment) Looks good but for one tiny thing... https://review.coreboot.org/c/flashrom/+/34160/4/dediprog.c File dediprog.c: https://review.coreboot.org/c/flashrom/+/34160/4/dediprog.c@1206 PS4, Line 1206: if (id != -1) { This is always true, right? `id` can't have changed since we checked this in line 1186, can it? Oh, now I see, it's just a left-over from the old version, isn't it? -- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 4 Gerrit-Owner: Ryan O'Leary <ryanoleary@google.com> Gerrit-Assignee: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Wed, 13 Nov 2019 13:23:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Hello ron minnich, David Hendricks, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/34160 to look at the new patch set (#5). Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... dediprog.c: Add id parameter to dediprog programmer When multiple dediprog programmers are connected, the 'id' parameter allows you to specify which one to use. The id is a string like SF012345 or DP012345. The value is printed on a sticker on the back of the dediprog. This is an improvement over the 'device' parameter which is based on enumeration order and changes when you plug/unplug devices or reboot the machine. To find the id without the sticker, run flashrom with the -V option. This prints the ids as they are enumerated. Alternatively, with dpcmd, you can use the --list-device-id and --fix-device commands to list and write device ids respectively. Note this only supports SF100 at the moment, but SF600 support is possible with more work. Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Signed-off-by: Ryan O'Leary <ryanoleary@google.com> --- M dediprog.c 1 file changed, 132 insertions(+), 21 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/60/34160/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 5 Gerrit-Owner: Ryan O'Leary <ryanoleary@google.com> Gerrit-Assignee: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Ryan O'Leary has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34160 ) Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... Patch Set 5: (1 comment) https://review.coreboot.org/c/flashrom/+/34160/4/dediprog.c File dediprog.c: https://review.coreboot.org/c/flashrom/+/34160/4/dediprog.c@1206 PS4, Line 1206: if (id != -1) {
This is always true, right? `id` can't have changed since we checked this in […] Yes, my mistake. Fixed!
-- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 5 Gerrit-Owner: Ryan O'Leary <ryanoleary@google.com> Gerrit-Assignee: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Wed, 13 Nov 2019 17:45:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34160 ) Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 5 Gerrit-Owner: Ryan O'Leary <ryanoleary@google.com> Gerrit-Assignee: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 14 Nov 2019 14:40:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Ryan O'Leary has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34160 ) Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... Patch Set 5: Hi Nico! Thanks for the review. Is there anything else needed to submit this? I don't have the permissions. -- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 5 Gerrit-Owner: Ryan O'Leary <ryanoleary@google.com> Gerrit-Assignee: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 15 Nov 2019 02:14:24 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34160 ) Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... Patch Set 5:
Hi Nico! Thanks for the review. Is there anything else needed to submit this? I don't have the permissions.
No, actually I wanted to submit it last night. Thanks for the ping. -- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 5 Gerrit-Owner: Ryan O'Leary <ryanoleary@google.com> Gerrit-Assignee: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 15 Nov 2019 09:29:33 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/34160 ) Change subject: dediprog.c: Add id parameter to dediprog programmer ...................................................................... dediprog.c: Add id parameter to dediprog programmer When multiple dediprog programmers are connected, the 'id' parameter allows you to specify which one to use. The id is a string like SF012345 or DP012345. The value is printed on a sticker on the back of the dediprog. This is an improvement over the 'device' parameter which is based on enumeration order and changes when you plug/unplug devices or reboot the machine. To find the id without the sticker, run flashrom with the -V option. This prints the ids as they are enumerated. Alternatively, with dpcmd, you can use the --list-device-id and --fix-device commands to list and write device ids respectively. Note this only supports SF100 at the moment, but SF600 support is possible with more work. Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Signed-off-by: Ryan O'Leary <ryanoleary@google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/34160 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Nico Huber <nico.h@gmx.de> --- M dediprog.c 1 file changed, 132 insertions(+), 21 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/dediprog.c b/dediprog.c index e4124b0..175e099 100644 --- a/dediprog.c +++ b/dediprog.c @@ -824,6 +824,33 @@ } /* + * Read the id from the dediprog. This should return the numeric part of the + * serial number found on a sticker on the back of the dediprog. Note this + * number is stored in writable eeprom, so it could get out of sync. Also note, + * this function only supports SF100 at this time, but SF600 support is not too + * much different. + * @return the id on success, -1 on failure + */ +static int dediprog_read_id(void) +{ + int ret; + uint8_t buf[3]; + + ret = libusb_control_transfer(dediprog_handle, REQTYPE_OTHER_IN, + 0x7, /* request */ + 0, /* value */ + 0xEF00, /* index */ + buf, sizeof(buf), + DEFAULT_TIMEOUT); + if (ret != sizeof(buf)) { + msg_perr("Failed to read dediprog id, error %d!\n", ret); + return -1; + } + + return buf[0] << 16 | buf[1] << 8 | buf[2]; +} + +/* * This command presumably sets the voltage for the SF100 itself (not the * SPI flash). Only use this command with firmware older than V6.0.0. Newer * (including all SF600s) do not support it. @@ -976,6 +1003,40 @@ .write_aai = dediprog_spi_write_aai, }; +/* + * Open a dediprog_handle with the USB device at the given index. + * @index index of the USB device + * @return 0 for success, -1 for error, -2 for busy device + */ +static int dediprog_open(int index) +{ + const uint16_t vid = devs_dediprog[0].vendor_id; + const uint16_t pid = devs_dediprog[0].device_id; + int ret; + + dediprog_handle = usb_dev_get_by_vid_pid_number(usb_ctx, vid, pid, (unsigned int) index); + if (!dediprog_handle) { + msg_perr("Could not find a Dediprog programmer on USB.\n"); + libusb_exit(usb_ctx); + return -1; + } + ret = libusb_set_configuration(dediprog_handle, 1); + if (ret != 0) { + msg_perr("Could not set USB device configuration: %i %s\n", + ret, libusb_error_name(ret)); + libusb_close(dediprog_handle); + return -2; + } + ret = libusb_claim_interface(dediprog_handle, 0); + if (ret < 0) { + msg_perr("Could not claim USB device interface %i: %i %s\n", + 0, ret, libusb_error_name(ret)); + libusb_close(dediprog_handle); + return -2; + } + return 0; +} + static int dediprog_shutdown(void *data) { dediprog_devicetype = DEV_UNKNOWN; @@ -996,9 +1057,11 @@ int dediprog_init(void) { - char *voltage, *device, *spispeed, *target_str; + char *voltage, *id_str, *device, *spispeed, *target_str; int spispeed_idx = 1; int millivolt = 3500; + int id = -1; /* -1 defaults to enumeration order */ + int found_id; long usedevice = 0; long target = FLASH_TYPE_APPLICATION_FLASH_1; int i, ret; @@ -1028,9 +1091,35 @@ msg_pinfo("Setting voltage to %i mV\n", millivolt); } + id_str = extract_programmer_param("id"); + if (id_str) { + char prefix0, prefix1; + if (sscanf(id_str, "%c%c%d", &prefix0, &prefix1, &id) != 3) { + msg_perr("Error: Could not parse dediprog 'id'.\n"); + msg_perr("Expected a string like SF012345 or DP012345.\n"); + free(id_str); + return 1; + } + if (id < 0 || id >= 0x1000000) { + msg_perr("Error: id %s is out of range!\n", id_str); + free(id_str); + return 1; + } + if (!(prefix0 == 'S' && prefix1 == 'F') && !(prefix0 == 'D' && prefix1 == 'P')) { + msg_perr("Error: %s is an invalid id!\n", id_str); + free(id_str); + return 1; + } + msg_pinfo("Will search for dediprog id %s.\n", id_str); + } + free(id_str); + device = extract_programmer_param("device"); if (device) { char *dev_suffix; + if (id != -1) { + msg_perr("Error: Cannot use 'id' and 'device'.\n"); + } errno = 0; usedevice = strtol(device, &dev_suffix, 10); if (errno != 0 || device == dev_suffix) { @@ -1094,27 +1183,49 @@ return 1; } - const uint16_t vid = devs_dediprog[0].vendor_id; - const uint16_t pid = devs_dediprog[0].device_id; - dediprog_handle = usb_dev_get_by_vid_pid_number(usb_ctx, vid, pid, (unsigned int) usedevice); - if (!dediprog_handle) { - msg_perr("Could not find a Dediprog programmer on USB.\n"); - libusb_exit(usb_ctx); - return 1; + if (id != -1) { + for (i = 0; ; i++) { + ret = dediprog_open(i); + if (ret == -1) { + /* no dev */ + libusb_exit(usb_ctx); + return 1; + } else if (ret == -2) { + /* busy dev */ + continue; + } + + /* Notice we can only call dediprog_read_id() after + * libusb_set_configuration() and + * libusb_claim_interface(). When searching by id and + * either configuration or claim fails (usually the + * device is in use by another instance of flashrom), + * the device is skipped and the next device is tried. + */ + found_id = dediprog_read_id(); + if (found_id < 0) { + msg_perr("Could not read id.\n"); + libusb_release_interface(dediprog_handle, 0); + libusb_close(dediprog_handle); + continue; + } + msg_pinfo("Found dediprog id SF%06d.\n", found_id); + if (found_id != id) { + libusb_release_interface(dediprog_handle, 0); + libusb_close(dediprog_handle); + continue; + } + break; + } + } else { + if (dediprog_open(usedevice)) { + return 1; + } + found_id = dediprog_read_id(); } - ret = libusb_set_configuration(dediprog_handle, 1); - if (ret != 0) { - msg_perr("Could not set USB device configuration: %i %s\n", ret, libusb_error_name(ret)); - libusb_close(dediprog_handle); - libusb_exit(usb_ctx); - return 1; - } - ret = libusb_claim_interface(dediprog_handle, 0); - if (ret < 0) { - msg_perr("Could not claim USB device interface %i: %i %s\n", 0, ret, libusb_error_name(ret)); - libusb_close(dediprog_handle); - libusb_exit(usb_ctx); - return 1; + + if (found_id >= 0) { + msg_pinfo("Using dediprog id SF%06d.\n", found_id); } if (register_shutdown(dediprog_shutdown, NULL)) -- To view, visit https://review.coreboot.org/c/flashrom/+/34160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4281213ab02131feb5d47bf66118a001cec0d219 Gerrit-Change-Number: 34160 Gerrit-PatchSet: 6 Gerrit-Owner: Ryan O'Leary <ryanoleary@google.com> Gerrit-Assignee: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Ryan O'Leary <ryanoleary@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Reviewer: ron minnich <rminnich@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: merged
participants (2)
-
Nico Huber (Code Review) -
Ryan O'Leary (Code Review)