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))
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).
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
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!
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
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?
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
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!
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
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.
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.
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))
-
Nico Huber (Code Review) -
Ryan O'Leary (Code Review)