Change in ...flashrom[master]: [WIP] picki2_spi: update to libusb1 and drop libusb0 dependence
Thomas Heijligen has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/32213
Change subject: [WIP] picki2_spi: update to libusb1 and drop libusb0 dependence ......................................................................
[WIP] picki2_spi: update to libusb1 and drop libusb0 dependence
Change-Id: Icfc5372aa1789d35ed22d68297d5e68a74d40388 Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com --- M Makefile M meson.build M meson_options.txt M pickit2_spi.c 4 files changed, 50 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/32213/1
diff --git a/Makefile b/Makefile index 1e7db3e..9d6123a 100644 --- a/Makefile +++ b/Makefile @@ -679,15 +679,12 @@ $(eval $(var)=yes))) endif
-# Disable feature groups -ifeq ($(CONFIG_ENABLE_LIBUSB0_PROGRAMMERS), no) -override CONFIG_PICKIT2_SPI = no -endif ifeq ($(CONFIG_ENABLE_LIBUSB1_PROGRAMMERS), no) override CONFIG_CH341A_SPI = no override CONFIG_DEDIPROG = no override CONFIG_DIGILENT_SPI = no override CONFIG_DEVELOPERBOX_SPI = no +override CONFIG_PICKIT2_SPI = no endif ifeq ($(CONFIG_ENABLE_LIBPCI_PROGRAMMERS), no) override CONFIG_INTERNAL = no @@ -852,7 +849,7 @@ ifeq ($(CONFIG_PICKIT2_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_PICKIT2_SPI=1' PROGRAMMER_OBJS += pickit2_spi.o -NEED_LIBUSB0 += CONFIG_PICKIT2_SPI +NEED_LIBUSB1 += CONFIG_PICKIT2_SPI endif
ifneq ($(NEED_LIBFTDI), ) @@ -1023,11 +1020,6 @@
endif
-ifneq ($(NEED_LIBUSB0), ) -CHECK_LIBUSB0 = yes -FEATURE_CFLAGS += -D'NEED_LIBUSB0=1' -USBLIBS := $(call debug_shell,[ -n "$(PKG_CONFIG_LIBDIR)" ] && export PKG_CONFIG_LIBDIR="$(PKG_CONFIG_LIBDIR)" ; $(PKG_CONFIG) --libs libusb || printf "%s" "-lusb") -endif
ifneq ($(NEED_LIBUSB1), ) CHECK_LIBUSB1 = yes @@ -1179,23 +1171,6 @@ endef export PCI_GET_DEV_TEST
-define LIBUSB0_TEST -#include "platform.h" -#if IS_WINDOWS -#include <lusb0_usb.h> -#else -#include <usb.h> -#endif -int main(int argc, char **argv) -{ - (void) argc; - (void) argv; - usb_init(); - return 0; -} -endef -export LIBUSB0_TEST - define LIBUSB1_TEST #include <stddef.h> #include <libusb.h> @@ -1261,28 +1236,6 @@ rm -f .test.c .test.o .test$(EXEC_SUFFIX); exit 1; }; }; } 2>>$(BUILD_DETAILS_FILE); echo $? >&3 ; } | tee -a $(BUILD_DETAILS_FILE) >&4; } 3>&1;} | { read rc ; exit ${rc}; } } 4>&1 @rm -f .test.c .test.o .test$(EXEC_SUFFIX) endif -ifeq ($(CHECK_LIBUSB0), yes) - @printf "Checking for libusb-0.1/libusb-compat headers... " | tee -a $(BUILD_DETAILS_FILE) - @echo "$$LIBUSB0_TEST" > .test.c - @printf "\nexec: %s\n" "$(CC) -c $(CPPFLAGS) $(CFLAGS) .test.c -o .test.o" >>$(BUILD_DETAILS_FILE) - @{ { { { { $(CC) -c $(CPPFLAGS) $(CFLAGS) .test.c -o .test.o >&2 && \ - echo "found." || { echo "not found."; echo; \ - echo "The following features require libusb-0.1/libusb-compat: $(NEED_LIBUSB0)."; \ - echo "Please install libusb-0.1 headers or libusb-compat headers or disable all features"; \ - echo "mentioned above by specifying make CONFIG_ENABLE_LIBUSB0_PROGRAMMERS=no"; \ - echo "See README for more information."; echo; \ - rm -f .test.c .test.o; exit 1; }; } 2>>$(BUILD_DETAILS_FILE); echo $? >&3 ; } | tee -a $(BUILD_DETAILS_FILE) >&4; } 3>&1;} | { read rc ; exit ${rc}; } } 4>&1 - @printf "Checking if libusb-0.1 is usable... " | tee -a $(BUILD_DETAILS_FILE) - @printf "\nexec: %s\n" "$(CC) $(LDFLAGS) .test.o -o .test$(EXEC_SUFFIX) $(LIBS) $(USBLIBS)" >>$(BUILD_DETAILS_FILE) - @{ { { { { $(CC) $(LDFLAGS) .test.o -o .test$(EXEC_SUFFIX) $(LIBS) $(USBLIBS) >&2 && \ - echo "yes." || { echo "no."; \ - echo "The following features require libusb-0.1/libusb-compat: $(NEED_LIBUSB0)."; \ - echo "Please install libusb-0.1 or libusb-compat or disable all features"; \ - echo "mentioned above by specifying make CONFIG_ENABLE_LIBUSB0_PROGRAMMERS=no"; \ - echo "See README for more information."; echo; \ - rm -f .test.c .test.o .test$(EXEC_SUFFIX); exit 1; }; } 2>>$(BUILD_DETAILS_FILE); echo $? >&3 ; } | tee -a $(BUILD_DETAILS_FILE) >&4; } 3>&1;} | { read rc ; exit ${rc}; } } 4>&1 - @rm -f .test.c .test.o .test$(EXEC_SUFFIX) -endif ifeq ($(CHECK_LIBUSB1), yes) @printf "Checking for libusb-1.0 headers... " | tee -a $(BUILD_DETAILS_FILE) @echo "$$LIBUSB1_TEST" > .test.c diff --git a/meson.build b/meson.build index 1923fdf..e1b6c16 100644 --- a/meson.build +++ b/meson.build @@ -67,7 +67,6 @@ deps = [] srcs = []
-need_libusb0 = false need_raw_access = false need_serial = false
@@ -91,6 +90,7 @@ config_dediprog = false config_digilent_spi = false config_developerbox_spi = false + config_pickit2_spi = false endif
# some programmers require libpci @@ -242,7 +242,6 @@ if config_pickit2_spi srcs += 'pickit2_spi.c' cargs += '-DCONFIG_PICKIT2_SPI=1' - need_libusb0 = true endif if config_pony_spi srcs += 'pony_spi.c' @@ -293,11 +292,6 @@ srcs += 'serial.c' endif
-# raw deprecated and old USB library -if need_libusb0 - deps += dependency('libusb') -endif - prefix = get_option('prefix') sbindir = join_paths(prefix, get_option('sbindir')) libdir = join_paths(prefix, get_option('libdir')) diff --git a/meson_options.txt b/meson_options.txt index 962866c..b7633d0 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -26,7 +26,7 @@ option('config_nicnatsemi', type : 'boolean', value : false, description : 'National Semiconductor NICs') option('config_nicrealtek', type : 'boolean', value : true, description : 'Realtek NICs') option('config_ogp_spi', type : 'boolean', value : true, description : 'SPI on OGP cards') -option('config_pickit2_spi', type : 'boolean', value : false, description : 'PICkit2 SPI') +option('config_pickit2_spi', type : 'boolean', value : true, description : 'PICkit2 SPI') option('config_pony_spi', type : 'boolean', value : true, description : 'PonyProg2000 SPI') option('config_rayer_spi', type : 'boolean', value : true, description : 'RayeR SPIPGM') option('config_satamv', type : 'boolean', value : true, description : 'Marvell SATA controllers') diff --git a/pickit2_spi.c b/pickit2_spi.c index 4354025..21a63f8 100644 --- a/pickit2_spi.c +++ b/pickit2_spi.c @@ -39,12 +39,7 @@ #include <string.h> #include <limits.h> #include <errno.h> - -#if IS_WINDOWS -#include <lusb0_usb.h> -#else -#include <usb.h> -#endif +#include <libusb.h>
#include "flash.h" #include "chipdrivers.h" @@ -57,7 +52,7 @@ {} };
-static usb_dev_handle *pickit2_handle; +static libusb_device_handle *pickit2_handle;
/* Default USB transaction timeout in ms */ #define DFLT_TIMEOUT 10000 @@ -94,36 +89,17 @@ #define SCR_VDD_OFF 0xFE #define SCR_VDD_ON 0xFF
-/* Might be useful for other USB devices as well. static for now. - * device parameter allows user to specify one device of multiple installed */ -static struct usb_device *get_device_by_vid_pid(uint16_t vid, uint16_t pid, unsigned int device) -{ - struct usb_bus *bus; - struct usb_device *dev; - - for (bus = usb_get_busses(); bus; bus = bus->next) - for (dev = bus->devices; dev; dev = dev->next) - if ((dev->descriptor.idVendor == vid) && - (dev->descriptor.idProduct == pid)) { - if (device == 0) - return dev; - device--; - } - - return NULL; -} - static int pickit2_get_firmware_version(void) { int ret; uint8_t command[CMD_LENGTH] = {CMD_GET_VERSION, CMD_END_OF_BUFFER};
- ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); - ret = usb_interrupt_read(pickit2_handle, ENDPOINT_IN, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); + ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, (unsigned char *)command, CMD_LENGTH, NULL, DFLT_TIMEOUT); + ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_IN, (unsigned char *)command, CMD_LENGTH, NULL, DFLT_TIMEOUT);
msg_pdbg("PICkit2 Firmware Version: %d.%d\n", (int)command[0], (int)command[1]); - if (ret != CMD_LENGTH) { - msg_perr("Command Get Firmware Version failed (%s)!\n", usb_strerror()); + if (ret != 0) { + msg_perr("Command Get Firmware Version failed!\n"); return 1; }
@@ -166,10 +142,10 @@ CMD_END_OF_BUFFER };
- int ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); + int ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, (unsigned char *)command, CMD_LENGTH, NULL, DFLT_TIMEOUT);
- if (ret != CMD_LENGTH) { - msg_perr("Command Set Voltage failed (%s)!\n", usb_strerror()); + if (ret != 0) { + msg_perr("Command Set Voltage failed!\n"); return 1; }
@@ -201,10 +177,10 @@ CMD_END_OF_BUFFER };
- int ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); + int ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, (unsigned char*)command, CMD_LENGTH, NULL, DFLT_TIMEOUT);
- if (ret != CMD_LENGTH) { - msg_perr("Command Set SPI Speed failed (%s)!\n", usb_strerror()); + if (ret != 0) { + msg_perr("Command Set SPI Speed failed!\n"); return 1; }
@@ -270,18 +246,19 @@ buf[i++] = CMD_UPLOAD_DATA; buf[i++] = CMD_END_OF_BUFFER;
- int ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)buf, CMD_LENGTH, DFLT_TIMEOUT); + int ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, (unsigned char *)buf, CMD_LENGTH, NULL, DFLT_TIMEOUT);
- if (ret != CMD_LENGTH) { - msg_perr("Send SPI failed, expected %i, got %i %s!\n", writecnt, ret, usb_strerror()); + if (ret != 0) { + msg_perr("Send SPI failed!\n"); return 1; }
if (readcnt) { - ret = usb_interrupt_read(pickit2_handle, ENDPOINT_IN, (char *)buf, CMD_LENGTH, DFLT_TIMEOUT); + int length = 0; + ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_IN, (unsigned char *)buf, CMD_LENGTH, &length, DFLT_TIMEOUT);
- if (ret != CMD_LENGTH) { - msg_perr("Receive SPI failed, expected %i, got %i %s!\n", readcnt, ret, usb_strerror()); + if (length == 0 || ret != 0) { + msg_perr("Receive SPI failed\n"); return 1; }
@@ -377,27 +354,22 @@ CMD_END_OF_BUFFER };
- int ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); + int ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, (unsigned char*)command, CMD_LENGTH, NULL, DFLT_TIMEOUT);
- if (ret != CMD_LENGTH) { - msg_perr("Command Shutdown failed (%s)!\n", usb_strerror()); + if (ret != 0) { + msg_perr("Command Shutdown failed (%s)!\n", libusb_strerror(ret)); ret = 1; } - if (usb_release_interface(pickit2_handle, 0) != 0) { + if (libusb_release_interface(pickit2_handle, 0) != 0) { msg_perr("Could not release USB interface!\n"); ret = 1; } - if (usb_close(pickit2_handle) != 0) { - msg_perr("Could not close USB device!\n"); - ret = 1; - } + libusb_close(pickit2_handle); return ret; }
int pickit2_spi_init(void) { - unsigned int usedevice = 0; // FIXME: Allow selecting one of multiple devices - uint8_t buf[CMD_LENGTH] = { CMD_EXEC_SCRIPT, 10, /* Script length */ @@ -444,31 +416,33 @@ }
/* Here comes the USB stuff */ - usb_init(); - (void)usb_find_busses(); - (void)usb_find_devices(); + if (pickit2_handle != NULL) { + // FIXME print error + return -1; + } + if (libusb_init(NULL) < 0) { + // FIXME print error + return -1; + } + + // FIXME libusb debug + const uint16_t vid = devs_pickit2_spi[0].vendor_id; const uint16_t pid = devs_pickit2_spi[0].device_id; - struct usb_device *dev = get_device_by_vid_pid(vid, pid, usedevice); - if (dev == NULL) { - msg_perr("Could not find a PICkit2 on USB!\n"); + pickit2_handle = libusb_open_device_with_vid_pid(NULL, vid, pid); + if (pickit2_handle == NULL) { + msg_perr("Could not open device PICkit2!\n"); return 1; } - msg_pdbg("Found USB device (%04x:%04x).\n", dev->descriptor.idVendor, dev->descriptor.idProduct);
- pickit2_handle = usb_open(dev); - int ret = usb_set_configuration(pickit2_handle, 1); - if (ret != 0) { - msg_perr("Could not set USB device configuration: %i %s\n", ret, usb_strerror()); - if (usb_close(pickit2_handle) != 0) - msg_perr("Could not close USB device!\n"); + if (libusb_set_configuration(pickit2_handle, 1) != 0) { + msg_perr("Could not set USB device configuration.\n"); + libusb_close(pickit2_handle); return 1; } - ret = usb_claim_interface(pickit2_handle, 0); - if (ret != 0) { - msg_perr("Could not claim USB device interface %i: %i %s\n", 0, ret, usb_strerror()); - if (usb_close(pickit2_handle) != 0) - msg_perr("Could not close USB device!\n"); + if (libusb_claim_interface(pickit2_handle, 0) != 0) { + msg_perr("Could not claim USB device interface\n"); + libusb_close(pickit2_handle); return 1; }
@@ -493,9 +467,8 @@
/* Perform basic setup. * Configure pin directions and logic levels, turn Vdd on, turn busy LED on and clear buffers. */ - ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)buf, CMD_LENGTH, DFLT_TIMEOUT); - if (ret != CMD_LENGTH) { - msg_perr("Command Setup failed (%s)!\n", usb_strerror()); + if (libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, (unsigned char *)buf, CMD_LENGTH, NULL, DFLT_TIMEOUT) != 0) { + msg_perr("Command Setup failed!\n"); return 1; }
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/32213
to look at the new patch set (#2).
Change subject: [WIP] picki2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
[WIP] picki2_spi: update to libusb1 and drop libusb0 dependency
Change-Id: Icfc5372aa1789d35ed22d68297d5e68a74d40388 Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com --- M Makefile M meson.build M meson_options.txt M pickit2_spi.c 4 files changed, 50 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/32213/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/32213
to look at the new patch set (#3).
Change subject: [WIP] pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
[WIP] pickit2_spi: update to libusb1 and drop libusb0 dependency
Change-Id: Icfc5372aa1789d35ed22d68297d5e68a74d40388 Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com --- M Makefile M meson.build M meson_options.txt M pickit2_spi.c 4 files changed, 50 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/32213/3
Hello Marius Genheimer, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/32213
to look at the new patch set (#4).
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
pickit2_spi: update to libusb1 and drop libusb0 dependency
TESTED: read, write, verify
Change-Id: Icfc5372aa1789d35ed22d68297d5e68a74d40388 Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com --- M Makefile M meson.build M meson_options.txt M pickit2_spi.c 4 files changed, 62 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/32213/4
Hello HAOUAS Elyes, Marius Genheimer, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/32213
to look at the new patch set (#5).
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
pickit2_spi: update to libusb1 and drop libusb0 dependency
TESTED: read, write, verify
Change-Id: Icfc5372aa1789d35ed22d68297d5e68a74d40388 Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com --- M Makefile M meson.build M meson_options.txt M pickit2_spi.c 4 files changed, 62 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/32213/5
Hello HAOUAS Elyes, Marius Genheimer, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/32213
to look at the new patch set (#6).
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
pickit2_spi: update to libusb1 and drop libusb0 dependency
TESTED: read, write, verify
Change-Id: Icfc5372aa1789d35ed22d68297d5e68a74d40388 Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com --- M Makefile M meson.build M meson_options.txt M pickit2_spi.c 4 files changed, 62 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/32213/6
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/32213 )
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
Patch Set 6: Code-Review+1
Marius Genheimer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/32213 )
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
Patch Set 6: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/32213 )
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
Patch Set 7:
(10 comments)
Looks good but I fear some things have to change. At least the `transferred` pointer to libusb_interrupt_transfer() and the missing libusb_exit().
If it's not too much trouble, it would be nice to retest, but only after the review is done. Doesn't have to cover a whole flash, a layout that covers two or more erase blocks with `--noverify-all` could speed things up. `--fmap` is also a nice test for nearly abitratry reads (if the FMAP is hard to find, i.e. not at a naturally aligned location).
Two things were noticed during review that should be fixed across all libusb programmers in the future:
* We shouldn't use the default context (passing NULL to libusb_init() etc.). It could be one libusb context per driver instance (flashrom_programmer_context), or maybe better a single one in a yet to be invented `flashrom_context` that is shared among driver instances. * We could always use libusb_strerror() in the code and implement our own (that simply returns the return code number as string, for instance) for older libusb versions, based on a version check.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c File pickit2_spi.c:
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@54 PS7, Line 54: #endif Not used here in this file? Then drop it.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@104 PS7, Line 104: (unsigned char *) This cast shouldn't be needed any more. And many more below...
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@104 PS7, Line 104: NULL NULL is only allowed since libusb 1.0.21. Sorry.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@366 PS7, Line 366: libusb_strerror It was only introduced 6 years ago, I guess that is why it's not used by any other driver. As you dropped most of these calls, let's not use it for now but keep it in mind.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@373 PS7, Line 373: libusb_close(pickit2_handle); libusb_exit(NULL);
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@428 PS7, Line 428: } Not a bug, could be a library user opening the device a second time. Just drop, please.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@430 PS7, Line 430: NULL NB. This will have to change in the future, but for all libusb drivers.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@445 PS7, Line 445: msg_perr("Could not open device PICkit2!\n"); libusb_exit(NULL);
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@451 PS7, Line 451: libusb_close(pickit2_handle); libusb_exit(NULL);
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@456 PS7, Line 456: libusb_close(pickit2_handle); libusb_exit(NULL);
Hello HAOUAS Elyes, Angel Pons, Marius Genheimer, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/32213
to look at the new patch set (#8).
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
pickit2_spi: update to libusb1 and drop libusb0 dependency
TESTED: read, write, verify
Change-Id: Icfc5372aa1789d35ed22d68297d5e68a74d40388 Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com --- M Makefile M meson.build M meson_options.txt M pickit2_spi.c 4 files changed, 59 insertions(+), 132 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/32213/8
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/32213 )
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
Patch Set 8:
(11 comments)
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c File pickit2_spi.c:
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@54 PS7, Line 54: #endif
Not used here in this file? Then drop it.
Ack
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@104 PS7, Line 104: NULL
NULL is only allowed since libusb 1.0.21. Sorry.
Ack
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@104 PS7, Line 104: (unsigned char *)
This cast shouldn't be needed any more. And many more below...
Ack
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@366 PS7, Line 366: libusb_strerror
It was only introduced 6 years ago, I guess that is why it's not […]
Ack
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@373 PS7, Line 373: libusb_close(pickit2_handle);
libusb_exit(NULL);
Ack
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@428 PS7, Line 428: }
Not a bug, could be a library user opening the device a second […]
Ack
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@445 PS7, Line 445: msg_perr("Could not open device PICkit2!\n");
libusb_exit(NULL);
Ack
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@451 PS7, Line 451: libusb_close(pickit2_handle);
libusb_exit(NULL);
Ack
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@456 PS7, Line 456: libusb_close(pickit2_handle);
libusb_exit(NULL);
Ack
https://review.coreboot.org/c/flashrom/+/32213/8/pickit2_spi.c File pickit2_spi.c:
https://review.coreboot.org/c/flashrom/+/32213/8/pickit2_spi.c@476 PS8, Line 476: (unsigned char *) :(
https://review.coreboot.org/c/flashrom/+/32213/8/pickit2_spi.c@476 PS8, Line 476: NULL :(
Hello HAOUAS Elyes, Angel Pons, Marius Genheimer, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/32213
to look at the new patch set (#9).
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
pickit2_spi: update to libusb1 and drop libusb0 dependency
TESTED: read, write, verify
Change-Id: Icfc5372aa1789d35ed22d68297d5e68a74d40388 Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com --- M Makefile M meson.build M meson_options.txt M pickit2_spi.c 4 files changed, 60 insertions(+), 132 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/32213/9
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/32213 )
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
Patch Set 9: Code-Review+2
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/flashrom/+/32213 )
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
pickit2_spi: update to libusb1 and drop libusb0 dependency
TESTED: read, write, verify
Change-Id: Icfc5372aa1789d35ed22d68297d5e68a74d40388 Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/32213 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M Makefile M meson.build M meson_options.txt M pickit2_spi.c 4 files changed, 60 insertions(+), 132 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/Makefile b/Makefile index 7a21d12..545e84a 100644 --- a/Makefile +++ b/Makefile @@ -690,15 +690,12 @@ $(eval $(var)=yes))) endif
-# Disable feature groups -ifeq ($(CONFIG_ENABLE_LIBUSB0_PROGRAMMERS), no) -override CONFIG_PICKIT2_SPI = no -endif ifeq ($(CONFIG_ENABLE_LIBUSB1_PROGRAMMERS), no) override CONFIG_CH341A_SPI = no override CONFIG_DEDIPROG = no override CONFIG_DIGILENT_SPI = no override CONFIG_DEVELOPERBOX_SPI = no +override CONFIG_PICKIT2_SPI = no endif ifeq ($(CONFIG_ENABLE_LIBPCI_PROGRAMMERS), no) override CONFIG_INTERNAL = no @@ -863,7 +860,7 @@ ifeq ($(CONFIG_PICKIT2_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_PICKIT2_SPI=1' PROGRAMMER_OBJS += pickit2_spi.o -NEED_LIBUSB0 += CONFIG_PICKIT2_SPI +NEED_LIBUSB1 += CONFIG_PICKIT2_SPI endif
ifneq ($(NEED_LIBFTDI), ) @@ -1034,11 +1031,6 @@
endif
-ifneq ($(NEED_LIBUSB0), ) -CHECK_LIBUSB0 = yes -FEATURE_CFLAGS += -D'NEED_LIBUSB0=1' -USBLIBS := $(call debug_shell,[ -n "$(PKG_CONFIG_LIBDIR)" ] && export PKG_CONFIG_LIBDIR="$(PKG_CONFIG_LIBDIR)" ; $(PKG_CONFIG) --libs libusb || printf "%s" "-lusb") -endif
ifneq ($(NEED_LIBUSB1), ) CHECK_LIBUSB1 = yes @@ -1190,23 +1182,6 @@ endef export PCI_GET_DEV_TEST
-define LIBUSB0_TEST -#include "platform.h" -#if IS_WINDOWS -#include <lusb0_usb.h> -#else -#include <usb.h> -#endif -int main(int argc, char **argv) -{ - (void) argc; - (void) argv; - usb_init(); - return 0; -} -endef -export LIBUSB0_TEST - define LIBUSB1_TEST #include <stddef.h> #include <libusb.h> @@ -1272,28 +1247,6 @@ rm -f .test.c .test.o .test$(EXEC_SUFFIX); exit 1; }; }; } 2>>$(BUILD_DETAILS_FILE); echo $? >&3 ; } | tee -a $(BUILD_DETAILS_FILE) >&4; } 3>&1;} | { read rc ; exit ${rc}; } } 4>&1 @rm -f .test.c .test.o .test$(EXEC_SUFFIX) endif -ifeq ($(CHECK_LIBUSB0), yes) - @printf "Checking for libusb-0.1/libusb-compat headers... " | tee -a $(BUILD_DETAILS_FILE) - @echo "$$LIBUSB0_TEST" > .test.c - @printf "\nexec: %s\n" "$(CC) -c $(CPPFLAGS) $(CFLAGS) .test.c -o .test.o" >>$(BUILD_DETAILS_FILE) - @{ { { { { $(CC) -c $(CPPFLAGS) $(CFLAGS) .test.c -o .test.o >&2 && \ - echo "found." || { echo "not found."; echo; \ - echo "The following features require libusb-0.1/libusb-compat: $(NEED_LIBUSB0)."; \ - echo "Please install libusb-0.1 headers or libusb-compat headers or disable all features"; \ - echo "mentioned above by specifying make CONFIG_ENABLE_LIBUSB0_PROGRAMMERS=no"; \ - echo "See README for more information."; echo; \ - rm -f .test.c .test.o; exit 1; }; } 2>>$(BUILD_DETAILS_FILE); echo $? >&3 ; } | tee -a $(BUILD_DETAILS_FILE) >&4; } 3>&1;} | { read rc ; exit ${rc}; } } 4>&1 - @printf "Checking if libusb-0.1 is usable... " | tee -a $(BUILD_DETAILS_FILE) - @printf "\nexec: %s\n" "$(CC) $(LDFLAGS) .test.o -o .test$(EXEC_SUFFIX) $(LIBS) $(USBLIBS)" >>$(BUILD_DETAILS_FILE) - @{ { { { { $(CC) $(LDFLAGS) .test.o -o .test$(EXEC_SUFFIX) $(LIBS) $(USBLIBS) >&2 && \ - echo "yes." || { echo "no."; \ - echo "The following features require libusb-0.1/libusb-compat: $(NEED_LIBUSB0)."; \ - echo "Please install libusb-0.1 or libusb-compat or disable all features"; \ - echo "mentioned above by specifying make CONFIG_ENABLE_LIBUSB0_PROGRAMMERS=no"; \ - echo "See README for more information."; echo; \ - rm -f .test.c .test.o .test$(EXEC_SUFFIX); exit 1; }; } 2>>$(BUILD_DETAILS_FILE); echo $? >&3 ; } | tee -a $(BUILD_DETAILS_FILE) >&4; } 3>&1;} | { read rc ; exit ${rc}; } } 4>&1 - @rm -f .test.c .test.o .test$(EXEC_SUFFIX) -endif ifeq ($(CHECK_LIBUSB1), yes) @printf "Checking for libusb-1.0 headers... " | tee -a $(BUILD_DETAILS_FILE) @echo "$$LIBUSB1_TEST" > .test.c diff --git a/meson.build b/meson.build index 1923fdf..e1b6c16 100644 --- a/meson.build +++ b/meson.build @@ -67,7 +67,6 @@ deps = [] srcs = []
-need_libusb0 = false need_raw_access = false need_serial = false
@@ -91,6 +90,7 @@ config_dediprog = false config_digilent_spi = false config_developerbox_spi = false + config_pickit2_spi = false endif
# some programmers require libpci @@ -242,7 +242,6 @@ if config_pickit2_spi srcs += 'pickit2_spi.c' cargs += '-DCONFIG_PICKIT2_SPI=1' - need_libusb0 = true endif if config_pony_spi srcs += 'pony_spi.c' @@ -293,11 +292,6 @@ srcs += 'serial.c' endif
-# raw deprecated and old USB library -if need_libusb0 - deps += dependency('libusb') -endif - prefix = get_option('prefix') sbindir = join_paths(prefix, get_option('sbindir')) libdir = join_paths(prefix, get_option('libdir')) diff --git a/meson_options.txt b/meson_options.txt index 962866c..b7633d0 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -26,7 +26,7 @@ option('config_nicnatsemi', type : 'boolean', value : false, description : 'National Semiconductor NICs') option('config_nicrealtek', type : 'boolean', value : true, description : 'Realtek NICs') option('config_ogp_spi', type : 'boolean', value : true, description : 'SPI on OGP cards') -option('config_pickit2_spi', type : 'boolean', value : false, description : 'PICkit2 SPI') +option('config_pickit2_spi', type : 'boolean', value : true, description : 'PICkit2 SPI') option('config_pony_spi', type : 'boolean', value : true, description : 'PonyProg2000 SPI') option('config_rayer_spi', type : 'boolean', value : true, description : 'RayeR SPIPGM') option('config_satamv', type : 'boolean', value : true, description : 'Marvell SATA controllers') diff --git a/pickit2_spi.c b/pickit2_spi.c index e13e721..6d9b28f 100644 --- a/pickit2_spi.c +++ b/pickit2_spi.c @@ -39,12 +39,7 @@ #include <string.h> #include <limits.h> #include <errno.h> - -#if IS_WINDOWS -#include <lusb0_usb.h> -#else -#include <usb.h> -#endif +#include <libusb.h>
#include "flash.h" #include "chipdrivers.h" @@ -57,7 +52,7 @@ {} };
-static usb_dev_handle *pickit2_handle; +static libusb_device_handle *pickit2_handle;
/* Default USB transaction timeout in ms */ #define DFLT_TIMEOUT 10000 @@ -94,36 +89,18 @@ #define SCR_VDD_OFF 0xFE #define SCR_VDD_ON 0xFF
-/* Might be useful for other USB devices as well. static for now. - * device parameter allows user to specify one device of multiple installed */ -static struct usb_device *get_device_by_vid_pid(uint16_t vid, uint16_t pid, unsigned int device) -{ - struct usb_bus *bus; - struct usb_device *dev; - - for (bus = usb_get_busses(); bus; bus = bus->next) - for (dev = bus->devices; dev; dev = dev->next) - if ((dev->descriptor.idVendor == vid) && - (dev->descriptor.idProduct == pid)) { - if (device == 0) - return dev; - device--; - } - - return NULL; -} - static int pickit2_get_firmware_version(void) { int ret; uint8_t command[CMD_LENGTH] = {CMD_GET_VERSION, CMD_END_OF_BUFFER};
- ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); - ret = usb_interrupt_read(pickit2_handle, ENDPOINT_IN, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); + int transferred; + ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT); + ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_IN, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT);
msg_pdbg("PICkit2 Firmware Version: %d.%d\n", (int)command[0], (int)command[1]); - if (ret != CMD_LENGTH) { - msg_perr("Command Get Firmware Version failed (%s)!\n", usb_strerror()); + if (ret != 0) { + msg_perr("Command Get Firmware Version failed!\n"); return 1; }
@@ -165,11 +142,11 @@ voltage_selector * 13, CMD_END_OF_BUFFER }; + int transferred; + int ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT);
- int ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); - - if (ret != CMD_LENGTH) { - msg_perr("Command Set Voltage failed (%s)!\n", usb_strerror()); + if (ret != 0) { + msg_perr("Command Set Voltage failed!\n"); return 1; }
@@ -201,10 +178,11 @@ CMD_END_OF_BUFFER };
- int ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); + int transferred; + int ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT);
- if (ret != CMD_LENGTH) { - msg_perr("Command Set SPI Speed failed (%s)!\n", usb_strerror()); + if (ret != 0) { + msg_perr("Command Set SPI Speed failed!\n"); return 1; }
@@ -270,18 +248,20 @@ buf[i++] = CMD_UPLOAD_DATA; buf[i++] = CMD_END_OF_BUFFER;
- int ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)buf, CMD_LENGTH, DFLT_TIMEOUT); + int transferred; + int ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, buf, CMD_LENGTH, &transferred, DFLT_TIMEOUT);
- if (ret != CMD_LENGTH) { - msg_perr("Send SPI failed, expected %i, got %i %s!\n", writecnt, ret, usb_strerror()); + if (ret != 0) { + msg_perr("Send SPI failed!\n"); return 1; }
if (readcnt) { - ret = usb_interrupt_read(pickit2_handle, ENDPOINT_IN, (char *)buf, CMD_LENGTH, DFLT_TIMEOUT); + int length = 0; + ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_IN, buf, CMD_LENGTH, &length, DFLT_TIMEOUT);
- if (ret != CMD_LENGTH) { - msg_perr("Receive SPI failed, expected %i, got %i %s!\n", readcnt, ret, usb_strerror()); + if (length == 0 || ret != 0) { + msg_perr("Receive SPI failed\n"); return 1; }
@@ -376,27 +356,24 @@ CMD_END_OF_BUFFER };
- int ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DFLT_TIMEOUT); + int transferred; + int ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT);
- if (ret != CMD_LENGTH) { - msg_perr("Command Shutdown failed (%s)!\n", usb_strerror()); + if (ret != 0) { + msg_perr("Command Shutdown failed!\n"); ret = 1; } - if (usb_release_interface(pickit2_handle, 0) != 0) { + if (libusb_release_interface(pickit2_handle, 0) != 0) { msg_perr("Could not release USB interface!\n"); ret = 1; } - if (usb_close(pickit2_handle) != 0) { - msg_perr("Could not close USB device!\n"); - ret = 1; - } + libusb_close(pickit2_handle); + libusb_exit(NULL); return ret; }
int pickit2_spi_init(void) { - unsigned int usedevice = 0; // FIXME: Allow selecting one of multiple devices - uint8_t buf[CMD_LENGTH] = { CMD_EXEC_SCRIPT, 10, /* Script length */ @@ -442,32 +419,36 @@ return 1; }
- /* Here comes the USB stuff */ - usb_init(); - (void)usb_find_busses(); - (void)usb_find_devices(); + if (libusb_init(NULL) < 0) { + msg_perr("Couldn't initialize libusb!\n"); + return -1; + } + +#if LIBUSB_API_VERSION < 0x01000106 + libusb_set_debug(NULL, 3); +#else + libusb_set_option(NULL, LIBUSB_OPTION_LOG_LEVEL, LIBUSB_LOG_LEVEL_INFO); +#endif + const uint16_t vid = devs_pickit2_spi[0].vendor_id; const uint16_t pid = devs_pickit2_spi[0].device_id; - struct usb_device *dev = get_device_by_vid_pid(vid, pid, usedevice); - if (dev == NULL) { - msg_perr("Could not find a PICkit2 on USB!\n"); + pickit2_handle = libusb_open_device_with_vid_pid(NULL, vid, pid); + if (pickit2_handle == NULL) { + msg_perr("Could not open device PICkit2!\n"); + libusb_exit(NULL); return 1; } - msg_pdbg("Found USB device (%04x:%04x).\n", dev->descriptor.idVendor, dev->descriptor.idProduct);
- pickit2_handle = usb_open(dev); - int ret = usb_set_configuration(pickit2_handle, 1); - if (ret != 0) { - msg_perr("Could not set USB device configuration: %i %s\n", ret, usb_strerror()); - if (usb_close(pickit2_handle) != 0) - msg_perr("Could not close USB device!\n"); + if (libusb_set_configuration(pickit2_handle, 1) != 0) { + msg_perr("Could not set USB device configuration.\n"); + libusb_close(pickit2_handle); + libusb_exit(NULL); return 1; } - ret = usb_claim_interface(pickit2_handle, 0); - if (ret != 0) { - msg_perr("Could not claim USB device interface %i: %i %s\n", 0, ret, usb_strerror()); - if (usb_close(pickit2_handle) != 0) - msg_perr("Could not close USB device!\n"); + if (libusb_claim_interface(pickit2_handle, 0) != 0) { + msg_perr("Could not claim USB device interface\n"); + libusb_close(pickit2_handle); + libusb_exit(NULL); return 1; }
@@ -492,9 +473,9 @@
/* Perform basic setup. * Configure pin directions and logic levels, turn Vdd on, turn busy LED on and clear buffers. */ - ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)buf, CMD_LENGTH, DFLT_TIMEOUT); - if (ret != CMD_LENGTH) { - msg_perr("Command Setup failed (%s)!\n", usb_strerror()); + int transferred; + if (libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, buf, CMD_LENGTH, &transferred, DFLT_TIMEOUT) != 0) { + msg_perr("Command Setup failed!\n"); return 1; }
-
HAOUAS Elyes (Code Review) -
Marius Genheimer (Code Review)
-
Nico Huber (Code Review)
-
Thomas Heijligen (Code Review)