Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
raiden_debug: Upstream ChromiumOS servo debug board prog
Initial check-in of the Raiden debugger programmer.
Squash in, usb_device: needed for raiden_debug prog
BUG=b:143389556 BRANCH=none TEST=builds
Change-Id: Ifad273a708acea4de797a0808be58960635a8864 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M Makefile M flashrom.c M meson.build M meson_options.txt M programmer.h A raiden_debug_spi.c A usb_device.c A usb_device.h 8 files changed, 977 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/09/38209/1
diff --git a/Makefile b/Makefile index 7242b09..e8ca7a8 100644 --- a/Makefile +++ b/Makefile @@ -226,6 +226,11 @@ else override CONFIG_RAYER_SPI = no endif +ifeq ($(CONFIG_RAIDEN), yes) +UNSUPPORTED_FEATURES += CONFIG_RAIDEN=yes +else +override CONFIG_RAIDEN = no +endif ifeq ($(CONFIG_NIC3COM), yes) UNSUPPORTED_FEATURES += CONFIG_NIC3COM=yes else @@ -607,6 +612,9 @@ # RayeR SPIPGM hardware support CONFIG_RAYER_SPI ?= yes
+# ChromiumOS servo DUT debug board hardware support +CONFIG_RAIDEN ?= yes + # PonyProg2000 SPI hardware support CONFIG_PONY_SPI ?= yes
@@ -817,6 +825,11 @@ NEED_RAW_ACCESS += CONFIG_RAYER_SPI endif
+ifeq ($(CONFIG_RAIDEN), yes) +FEATURE_CFLAGS += -D'CONFIG_RAIDEN=1' +PROGRAMMER_OBJS += raiden_debug_spi.o usb_device.o +endif + ifeq ($(CONFIG_PONY_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_PONY_SPI=1' PROGRAMMER_OBJS += pony_spi.o diff --git a/flashrom.c b/flashrom.c index e540027..e541b68 100644 --- a/flashrom.c +++ b/flashrom.c @@ -133,6 +133,18 @@ }, #endif
+#if CONFIG_RAIDEN == 1 + { + .name = "raiden_debug", + .type = USB, + .devs.note = "All programmer devices speaking the raiden protocol\n", + .init = raiden_debug_spi_init, + .map_flash_region = fallback_map, + .unmap_flash_region = fallback_unmap, + .delay = internal_delay, + }, +#endif + #if CONFIG_DRKAISER == 1 { .name = "drkaiser", diff --git a/meson.build b/meson.build index 375089c..88b14cf 100644 --- a/meson.build +++ b/meson.build @@ -42,6 +42,7 @@ config_dummy = get_option('config_dummy') config_ft2232_spi = get_option('config_ft2232_spi') config_gfxnvidia = get_option('config_gfxnvidia') +config_raiden = get_option('config_raiden') config_internal = get_option('config_internal') config_it8212 = get_option('config_it8212') config_linux_mtd = get_option('config_linux_mtd') @@ -104,6 +105,7 @@ config_atavia = false config_drkaiser = false config_gfxnvidia = false + config_raiden = false config_internal = false config_it8212 = false config_nic3com = false @@ -170,6 +172,11 @@ srcs += 'gfxnvidia.c' cargs += '-DCONFIG_GFXNVIDIA=1' endif +if config_raiden + srcs += 'raiden_debug_spi.c' + srcs += 'usb_device.c' + cargs += '-DCONFIG_RAIDEN=1' +endif if config_internal srcs += 'board_enable.c' srcs += 'cbtable.c' diff --git a/meson_options.txt b/meson_options.txt index b7633d0..4334814 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -13,6 +13,7 @@ option('config_dummy', type : 'boolean', value : true, description : 'dummy tracing') option('config_ft2232_spi', type : 'boolean', value : true, description : 'FT2232 SPI dongles') option('config_gfxnvidia', type : 'boolean', value : true, description : 'NVIDIA graphics cards') +option('config_raiden', type : 'boolean', value : true, description : 'ChromiumOS Servo DUT debug board') option('config_internal', type : 'boolean', value : true, description : 'internal/onboard') option('config_internal_dmi', type : 'boolean', value : true, description : 'Use internal DMI parser') option('config_it8212', type : 'boolean', value : true, description : 'ITE IT8212F PATA') diff --git a/programmer.h b/programmer.h index 3cf53b9..08500c6 100644 --- a/programmer.h +++ b/programmer.h @@ -43,6 +43,9 @@ #if CONFIG_GFXNVIDIA == 1 PROGRAMMER_GFXNVIDIA, #endif +#if CONFIG_RAIDEN == 1 + PROGRAMMER_RAIDEN, +#endif #if CONFIG_DRKAISER == 1 PROGRAMMER_DRKAISER, #endif @@ -401,6 +404,11 @@ extern const struct dev_entry gfx_nvidia[]; #endif
+/* raiden_debug_spi.c */ +#if CONFIG_RAIDEN == 1 +int raiden_debug_spi_init(void); +#endif + /* drkaiser.c */ #if CONFIG_DRKAISER == 1 int drkaiser_init(void); diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c new file mode 100644 index 0000000..9af0f7d --- /dev/null +++ b/raiden_debug_spi.c @@ -0,0 +1,369 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2014, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * Alternatively, this software may be distributed under the terms of the + * GNU General Public License ("GPL") version 2 as published by the Free + * Software Foundation. + */ + +/* + * This SPI flash programming interface is designed to talk to a Chromium OS + * device over a Raiden USB connection. The USB connection is routed to a + * microcontroller running an image compiled from: + * + * https://chromium.googlesource.com/chromiumos/platform/ec + * + * The protocol for the USB-SPI bridge is documented in the following file in + * that respository: + * + * chip/stm32/usb_spi.c + */ + +#include "programmer.h" +#include "spi.h" +#include "usb_device.h" + +#include <libusb.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#define GOOGLE_VID 0x18D1 +#define GOOGLE_RAIDEN_SPI_SUBCLASS 0x51 +#define GOOGLE_RAIDEN_SPI_PROTOCOL 0x01 + +enum raiden_debug_spi_request { + RAIDEN_DEBUG_SPI_REQ_ENABLE = 0x0000, + RAIDEN_DEBUG_SPI_REQ_DISABLE = 0x0001, + RAIDEN_DEBUG_SPI_REQ_ENABLE_AP = 0x0002, + RAIDEN_DEBUG_SPI_REQ_ENABLE_EC = 0x0003, +}; + +#define PACKET_HEADER_SIZE 2 +#define MAX_PACKET_SIZE 64 + +/* + * This timeout is so large because the Raiden SPI timeout is 800ms. + */ +#define TRANSFER_TIMEOUT_MS 1000 + +struct usb_device *device = NULL; +uint8_t in_endpoint = 0; +uint8_t out_endpoint = 0; + +static int send_command(struct flashctx *flash, + unsigned int write_count, + unsigned int read_count, + const unsigned char *write_buffer, + unsigned char *read_buffer) +{ + uint8_t buffer[MAX_PACKET_SIZE]; + int transferred; + + if (write_count > MAX_PACKET_SIZE - PACKET_HEADER_SIZE) { + msg_perr("Raiden: invalid write_count of %d\n", write_count); + return SPI_INVALID_LENGTH; + } + + if (read_count > MAX_PACKET_SIZE - PACKET_HEADER_SIZE) { + msg_perr("Raiden: invalid read_count of %d\n", read_count); + return SPI_INVALID_LENGTH; + } + + buffer[0] = write_count; + buffer[1] = read_count; + + memcpy(buffer + PACKET_HEADER_SIZE, write_buffer, write_count); + + CHECK(LIBUSB(libusb_bulk_transfer(device->handle, + out_endpoint, + buffer, + write_count + PACKET_HEADER_SIZE, + &transferred, + TRANSFER_TIMEOUT_MS)), + "Raiden: OUT transfer failed\n" + " write_count = %d\n" + " read_count = %d\n", + write_count, + read_count); + + if ((unsigned) transferred != write_count + PACKET_HEADER_SIZE) { + msg_perr("Raiden: Write failure (wrote %d, expected %d)\n", + transferred, write_count + PACKET_HEADER_SIZE); + return 0x10001; + } + + CHECK(LIBUSB(libusb_bulk_transfer(device->handle, + in_endpoint, + buffer, + read_count + PACKET_HEADER_SIZE, + &transferred, + TRANSFER_TIMEOUT_MS)), + "Raiden: IN transfer failed\n" + " write_count = %d\n" + " read_count = %d\n", + write_count, + read_count); + + if ((unsigned) transferred != read_count + PACKET_HEADER_SIZE) { + msg_perr("Raiden: Read failure (read %d, expected %d)\n", + transferred, read_count + PACKET_HEADER_SIZE); + return 0x10002; + } + + memcpy(read_buffer, buffer + PACKET_HEADER_SIZE, read_count); + + return buffer[0] | (buffer[1] << 8); +} + +/* + * Unfortunately there doesn't seem to be a way to specify the maximum number + * of bytes that your SPI device can read/write, these values are the maximum + * data chunk size that flashrom will package up with an additional five bytes + * of command for the flash device, resulting in a 62 byte packet, that we then + * add two bytes to in either direction, making our way up to the 64 byte + * maximum USB packet size for the device. + * + * The largest command that flashrom generates is the byte program command, so + * we use that command header maximum size here. + */ +#define MAX_DATA_SIZE (MAX_PACKET_SIZE - \ + PACKET_HEADER_SIZE - \ + JEDEC_BYTE_PROGRAM_OUTSIZE) + +static const struct spi_master spi_master_raiden_debug = { + .features = SPI_MASTER_4BA, + .max_data_read = MAX_DATA_SIZE, + .max_data_write = MAX_DATA_SIZE, + .command = send_command, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, +}; + +static int match_endpoint(struct libusb_endpoint_descriptor const *descriptor, + enum libusb_endpoint_direction direction) +{ + return (((descriptor->bEndpointAddress & LIBUSB_ENDPOINT_DIR_MASK) == + direction) && + ((descriptor->bmAttributes & LIBUSB_TRANSFER_TYPE_MASK) == + LIBUSB_TRANSFER_TYPE_BULK)); +} + +static int find_endpoints(struct usb_device *dev, uint8_t *in_ep, uint8_t *out_ep) +{ + int i; + int in_count = 0; + int out_count = 0; + + for (i = 0; i < dev->interface_descriptor->bNumEndpoints; i++) { + struct libusb_endpoint_descriptor const *endpoint = + &dev->interface_descriptor->endpoint[i]; + + if (match_endpoint(endpoint, LIBUSB_ENDPOINT_IN)) { + in_count++; + *in_ep = endpoint->bEndpointAddress; + } else if (match_endpoint(endpoint, LIBUSB_ENDPOINT_OUT)) { + out_count++; + *out_ep = endpoint->bEndpointAddress; + } + } + + if (in_count != 1 || out_count != 1) { + msg_perr("Raiden: Failed to find one IN and one OUT endpoint\n" + " found %d IN and %d OUT endpoints\n", + in_count, + out_count); + return 1; + } + + msg_pdbg("Raiden: Found IN endpoint = 0x%02x\n", *in_ep); + msg_pdbg("Raiden: Found OUT endpoint = 0x%02x\n", *out_ep); + + return 0; +} + +static int shutdown(void * data) +{ + CHECK(LIBUSB(libusb_control_transfer( + device->handle, + LIBUSB_ENDPOINT_OUT | + LIBUSB_REQUEST_TYPE_VENDOR | + LIBUSB_RECIPIENT_INTERFACE, + RAIDEN_DEBUG_SPI_REQ_DISABLE, + 0, + device->interface_descriptor->bInterfaceNumber, + NULL, + 0, + TRANSFER_TIMEOUT_MS)), + "Raiden: Failed to disable SPI bridge\n"); + + usb_device_free(device); + + device = NULL; + libusb_exit(NULL); + + return 0; +} + +static int get_target() +{ + int request_enable = RAIDEN_DEBUG_SPI_REQ_ENABLE; + + char *target_str = extract_programmer_param("target"); + if (target_str) { + if (!strcasecmp(target_str, "ap")) + request_enable = RAIDEN_DEBUG_SPI_REQ_ENABLE_AP; + else if (!strcasecmp(target_str, "ec")) + request_enable = RAIDEN_DEBUG_SPI_REQ_ENABLE_EC; + else { + msg_perr("Invalid target: %s\n", target_str); + request_enable = -1; + } + } + free(target_str); + + return request_enable; +} + +static void free_dev_list(struct usb_device **dev_lst) +{ + struct usb_device *dev = *dev_lst; + /* free devices we don't care about */ + dev = dev->next; + while (dev) + dev = usb_device_free(dev); +} + +int raiden_debug_spi_init(void) +{ + struct usb_match match; + char *serial = extract_programmer_param("serial"); + struct usb_device *current; + int found = 0; + + int request_enable = get_target(); + if (request_enable < 0) + return 1; + + usb_match_init(&match); + + usb_match_value_default(&match.vid, GOOGLE_VID); + usb_match_value_default(&match.class, LIBUSB_CLASS_VENDOR_SPEC); + usb_match_value_default(&match.subclass, GOOGLE_RAIDEN_SPI_SUBCLASS); + usb_match_value_default(&match.protocol, GOOGLE_RAIDEN_SPI_PROTOCOL); + + CHECK(LIBUSB(libusb_init(NULL)), "Raiden: libusb_init failed\n"); + + CHECK(usb_device_find(&match, ¤t), + "Raiden: Failed to find devices\n"); + + while (current) { + device = current; + + if (find_endpoints(device, &in_endpoint, &out_endpoint)) { + msg_pdbg("Raiden: Failed to find valid endpoints on device"); + usb_device_show(" ", current); + goto loop_end; + } + + if (usb_device_claim(device)) { + msg_pdbg("Raiden: Failed to claim USB device"); + usb_device_show(" ", current); + goto loop_end; + } + + if (!serial) { + found = 1; + goto loop_end; + } else { + unsigned char dev_serial[32]; + struct libusb_device_descriptor descriptor; + int rc; + + memset(dev_serial, 0, sizeof(dev_serial)); + + if (libusb_get_device_descriptor(device->device, &descriptor)) { + msg_pdbg("USB: Failed to get device descriptor.\n"); + goto loop_end; + } + + rc = libusb_get_string_descriptor_ascii(device->handle, + descriptor.iSerialNumber, + dev_serial, + sizeof(dev_serial)); + if (rc < 0) { + LIBUSB(rc); + } else { + if (strcmp(serial, (char *)dev_serial)) { + msg_pdbg("Raiden: Serial number %s did not match device", serial); + usb_device_show(" ", current); + } else { + msg_pinfo("Raiden: Serial number %s matched device", serial); + usb_device_show(" ", current); + found = 1; + } + } + } + +loop_end: + if (found) + break; + else + current = usb_device_free(current); + } + + if (!device || !found) { + msg_perr("Raiden: No usable device found.\n"); + return 1; + } + + free_dev_list(¤t); + + CHECK(LIBUSB(libusb_control_transfer( + device->handle, + LIBUSB_ENDPOINT_OUT | + LIBUSB_REQUEST_TYPE_VENDOR | + LIBUSB_RECIPIENT_INTERFACE, + request_enable, + 0, + device->interface_descriptor->bInterfaceNumber, + NULL, + 0, + TRANSFER_TIMEOUT_MS)), + "Raiden: Failed to enable SPI bridge\n"); + + register_spi_master(&spi_master_raiden_debug); + register_shutdown(shutdown, NULL); + + return 0; +} diff --git a/usb_device.c b/usb_device.c new file mode 100644 index 0000000..0909694 --- /dev/null +++ b/usb_device.c @@ -0,0 +1,366 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2015, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * Alternatively, this software may be distributed under the terms of the + * GNU General Public License ("GPL") version 2 as published by the Free + * Software Foundation. + */ + +#include "programmer.h" +#include "spi.h" +#include "usb_device.h" + +#include <assert.h> +#include <libusb.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +/* + * Possibly extract a programmer parameter and use it to initialize the given + * match value structure. + */ +static void usb_match_value_init(struct usb_match_value *match, + char const *parameter) +{ + char *string = extract_programmer_param(parameter); + + match->name = parameter; + + if (string) { + match->set = 1; + match->value = strtol(string, NULL, 0); + } else { + match->set = 0; + } + + free(string); +} + +#define USB_MATCH_VALUE_INIT(NAME) \ + usb_match_value_init(&match->NAME, #NAME) + +void usb_match_init(struct usb_match *match) +{ + USB_MATCH_VALUE_INIT(vid); + USB_MATCH_VALUE_INIT(pid); + USB_MATCH_VALUE_INIT(bus); + USB_MATCH_VALUE_INIT(address); + USB_MATCH_VALUE_INIT(config); + USB_MATCH_VALUE_INIT(interface); + USB_MATCH_VALUE_INIT(altsetting); + USB_MATCH_VALUE_INIT(class); + USB_MATCH_VALUE_INIT(subclass); + USB_MATCH_VALUE_INIT(protocol); +} + +void usb_match_value_default(struct usb_match_value *value, + long int default_value) +{ + if (value->set) + return; + + value->set = 1; + value->value = default_value; +} + +/* + * Match the value against a possible user supplied parameter. + * + * Return: + * 0: The user supplied the given parameter and it did not match the value. + * 1: Either the user didn't supply the parameter, or they did and it + * matches the given value. + */ +static int check_match(struct usb_match_value const *match_value, int value) +{ + int reject = match_value->set && (match_value->value != value); + + if (reject) + msg_pdbg("USB: Rejecting device because %s = %d != %d\n", + match_value->name, + value, + match_value->value); + + return !reject; +} + +/* + * Allocate a copy of device and add it to the head of the devices list. + */ +static void add_device(struct usb_device *device, + struct usb_device **devices) +{ + struct usb_device *copy = malloc(sizeof(struct usb_device)); + + assert(copy != NULL); + + *copy = *device; + + copy->next = *devices; + *devices = copy; + + libusb_ref_device(copy->device); +} + +/* + * Look through the interfaces of the current device config for a match. Stop + * looking after the first valid match is found. + * + * Return: + * 0: No matching interface was found. + * 1: A complete match was found and added to the devices list. + */ +static int find_interface(struct usb_match const *match, + struct usb_device *current, + struct usb_device **devices) +{ + int i, j; + + for (i = 0; i < current->config_descriptor->bNumInterfaces; ++i) { + struct libusb_interface const *interface; + + interface = ¤t->config_descriptor->interface[i]; + + for (j = 0; j < interface->num_altsetting; ++j) { + struct libusb_interface_descriptor const *descriptor; + + descriptor = &interface->altsetting[j]; + + if (check_match(&match->interface, + descriptor->bInterfaceNumber) && + check_match(&match->altsetting, + descriptor->bAlternateSetting) && + check_match(&match->class, + descriptor->bInterfaceClass) && + check_match(&match->subclass, + descriptor->bInterfaceSubClass) && + check_match(&match->protocol, + descriptor->bInterfaceProtocol)) { + current->interface_descriptor = descriptor; + add_device(current, devices); + msg_pdbg("USB: Found matching device\n"); + return 1; + } + } + } + + return 0; +} + +/* + * Look through the configs of the current device for a match. Stop looking + * after the first valid match is found. + * + * Return: + * 0: All configurations successfully checked, one may have been added to + * the list. + * non-zero: There was a failure while checking for a match. + */ +static int find_config(struct usb_match const *match, + struct usb_device *current, + struct libusb_device_descriptor const *device_descriptor, + struct usb_device **devices) +{ + int i; + + for (i = 0; i < device_descriptor->bNumConfigurations; ++i) { + CHECK(LIBUSB(libusb_get_config_descriptor( + current->device, + i, + ¤t->config_descriptor)), + "USB: Failed to get config descriptor"); + + if (check_match(&match->config, + current->config_descriptor-> + bConfigurationValue) && + find_interface(match, current, devices)) + break; + + libusb_free_config_descriptor(current->config_descriptor); + } + + return 0; +} + +int usb_device_find(struct usb_match const *match, struct usb_device **devices) +{ + libusb_device **list; + size_t count; + size_t i; + + *devices = NULL; + + CHECK(LIBUSB(count = libusb_get_device_list(NULL, &list)), + "USB: Failed to get device list"); + + for (i = 0; i < count; ++i) { + struct libusb_device_descriptor descriptor; + struct usb_device current = { + .device = list[i], + .handle = NULL, + .next = NULL, + }; + + uint8_t bus = libusb_get_bus_number(list[i]); + uint8_t address = libusb_get_device_address(list[i]); + + msg_pdbg("USB: Inspecting device (Bus %d, Address %d)\n", + bus, + address); + + CHECK(LIBUSB(libusb_get_device_descriptor(list[i], + &descriptor)), + "USB: Failed to get device descriptor"); + + if (check_match(&match->vid, descriptor.idVendor) && + check_match(&match->pid, descriptor.idProduct) && + check_match(&match->bus, bus) && + check_match(&match->address, address)) + CHECK(find_config(match, + ¤t, + &descriptor, + devices), + "USB: Failed to find config"); + } + + libusb_free_device_list(list, 1); + + return (*devices == NULL); +} + +/* + * If the underlying libusb device is not open, open it. + * + * Return: + * 0: The device was already open or was successfully opened. + * non-zero: There was a failure while opening the device. + */ +static int usb_device_open(struct usb_device *device) +{ + if (device->handle == NULL) + CHECK(LIBUSB(libusb_open(device->device, &device->handle)), + "USB: Failed to open device\n"); + + return 0; +} + +int usb_device_show(char const *prefix, struct usb_device *device) +{ + struct libusb_device_descriptor descriptor; + unsigned char product[256]; + + CHECK(usb_device_open(device), "USB: Failed to open device\n"); + + CHECK(LIBUSB(libusb_get_device_descriptor(device->device, &descriptor)), + "USB: Failed to get device descriptor\n"); + + CHECK(LIBUSB(libusb_get_string_descriptor_ascii( + device->handle, + descriptor.iProduct, + product, + sizeof(product))), + "USB: Failed to get device product string\n"); + + product[255] = '\0'; + + msg_perr("%sbus=0x%02x,address=0x%02x | %s\n", + prefix, + libusb_get_bus_number(device->device), + libusb_get_device_address(device->device), + product); + + return 0; +} + +int usb_device_claim(struct usb_device *device) +{ + int current_config; + + CHECK(usb_device_open(device), "USB: Failed to open device\n"); + + CHECK(LIBUSB(libusb_get_configuration(device->handle, + ¤t_config)), + "USB: Failed to get current device configuration\n"); + + if (current_config != device->config_descriptor->bConfigurationValue) + CHECK(LIBUSB(libusb_set_configuration( + device->handle, + device-> + config_descriptor-> + bConfigurationValue)), + "USB: Failed to set new configuration from %d to %d\n", + current_config, + device->config_descriptor->bConfigurationValue); + + CHECK(LIBUSB(libusb_set_auto_detach_kernel_driver(device->handle, 1)), + "USB: Failed to enable auto kernel driver detach\n"); + + CHECK(LIBUSB(libusb_claim_interface(device->handle, + device-> + interface_descriptor-> + bInterfaceNumber)), + "USB: Could not claim device interface %d\n", + device->interface_descriptor->bInterfaceNumber); + + if (device->interface_descriptor->bAlternateSetting != 0) + CHECK(LIBUSB(libusb_set_interface_alt_setting( + device->handle, + device-> + interface_descriptor-> + bInterfaceNumber, + device-> + interface_descriptor-> + bAlternateSetting)), + "USB: Failed to set alternate setting %d\n", + device->interface_descriptor->bAlternateSetting); + + return 0; +} + +struct usb_device *usb_device_free(struct usb_device *device) +{ + struct usb_device *next = device->next; + + if (device->handle != NULL) + libusb_close(device->handle); + + /* + * This unref balances the ref added in the add_device function. + */ + libusb_unref_device(device->device); + libusb_free_config_descriptor(device->config_descriptor); + + free(device); + + return next; +} diff --git a/usb_device.h b/usb_device.h new file mode 100644 index 0000000..46d2458 --- /dev/null +++ b/usb_device.h @@ -0,0 +1,201 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2015, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * Alternatively, this software may be distributed under the terms of the + * GNU General Public License ("GPL") version 2 as published by the Free + * Software Foundation. + */ + +/* + * USB device matching framework + * + * This can be used to match a USB device by a number of different parameters. + * The parameters can be passed on the command line and defaults can be set + * by the programmer. + */ + +#include <libusb.h> +#include <stdint.h> + +/* + * Check Macro + * + * The check macro simplifies common return error code checking logic. If the + * expression does not evaluate to zero then the string error is printed and + * the expressions result is imediately returned, else the result is available + * as the value of the CHECK expression statement. + */ + +#define CHECK(expression, string...) \ + ({ \ + int error__ = (expression); \ + \ + if (error__ != 0) { \ + msg_perr(string); \ + return error__; \ + } \ + \ + error__; \ + }) + +/* + * The LIBUSB macro converts a libusb failure code into an error code that + * flashrom recognizes. It also displays additional libusb specific + * information about the failure. + */ +#define LIBUSB(expression) \ + ({ \ + int libusb_error__ = (expression); \ + \ + if (libusb_error__ < 0) { \ + msg_perr("libusb error: %s:%d %s\n", \ + __FILE__, \ + __LINE__, \ + libusb_error_name(libusb_error__)); \ + libusb_error__ = 0x20000 | -libusb_error__; \ + } else { \ + libusb_error__ = 0; \ + } \ + \ + libusb_error__; \ + }) + +/* + * A USB match and associated value struct are used to encode the information + * about a device against which we wish to match. If the value of a + * usb_match_value has been set then a device must match that value. The name + * of the usb_match_value is used to fetch the programmer parameter from the + * flashrom command line and is the same as the name of the corresponding + * field in usb_match. + */ +struct usb_match_value { + char const *name; + int value; + int set; +}; + +struct usb_match { + struct usb_match_value bus; + struct usb_match_value address; + struct usb_match_value vid; + struct usb_match_value pid; + struct usb_match_value serial; + struct usb_match_value config; + struct usb_match_value interface; + struct usb_match_value altsetting; + struct usb_match_value class; + struct usb_match_value subclass; + struct usb_match_value protocol; +}; + +/* + * Initialize a usb_match structure so that each value's name matches the + * values name in the usb_match structure (so bus.name == "bus"...), and + * look for each value in the flashrom command line via + * extract_programmer_param. If the value is found convert it to an integer + * using strtol, accepting hex, decimal and octal encoding. + */ +void usb_match_init(struct usb_match *match); + +/* + * Add a default value to a usb_match_value. This must be done after calling + * usb_match_init. If usb_match_init already set the value of a usb_match_value + * we do nothing, otherwise set the value to default_value. This ensures that + * parameters passed on the command line override defaults. + */ +void usb_match_value_default(struct usb_match_value *match, + long int default_value); + +/* + * The usb_device structure is an entry in a linked list of devices that were + * matched by usb_device_find. + */ +struct usb_device { + struct libusb_device *device; + struct libusb_config_descriptor *config_descriptor; + struct libusb_interface_descriptor const *interface_descriptor; + + /* + * Initially NULL, the libusb_device_handle is only valid once the + * usb_device has been successfully passed to usb_device_show or + * usb_device_claim. + */ + struct libusb_device_handle *handle; + + /* + * Link to next device, or NULL + */ + struct usb_device *next; +}; + +/* + * Find and return a list of all compatible devices. Each device is added to + * the list with its first valid configuration and interface. If an alternate + * configuration (config, interface, altsetting...) is desired the specifics + * can be supplied as programmer parameters. + * + * Return: + * 0: At least one matching device was found. + * 1: No matching devices were found. + */ +int usb_device_find(struct usb_match const *match, struct usb_device **devices); + +/* + * Display the devices bus and address as well as its product string. The + * underlying libusb device is opened if it is not already open. + * + * Return: + * 0: The device information was displayed. + * non-zero: There was a failure while displaying the device information. + */ +int usb_device_show(char const *prefix, struct usb_device *device); + +/* + * Open the underlying libusb device, set its config, claim the interface and + * select the correct alternate interface. + * + * Return: + * 0: The device was successfully claimed. + * non-zero: There was a failure while trying to claim the device. + */ +int usb_device_claim(struct usb_device *device); + +/* + * Free a usb_device structure. + * + * This ensures that the libusb device is closed and that all allocated + * handles and descriptors are freed. + * + * Return: + * The next device in the device list. + */ +struct usb_device *usb_device_free(struct usb_device *device);
Hello Angel Pons, build bot (Jenkins), Vadim Bendebury, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/38209
to look at the new patch set (#2).
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
raiden_debug: Upstream ChromiumOS servo debug board prog
Initial check-in of the Raiden debugger programmer for the ChromiumOS servo debug tool. This then allows upstream flashrom to fully support the servo debug hardware tool.
While it isn't ideal to have usb_device.[h|c] when usbdev.c already exists in-tree, we get the initial raiden_debug supporting code in so we can massage it moving forwards.
Squash in, usb_device: needed for raiden_debug prog
BUG=b:143389556 BRANCH=none TEST=builds && ran `./flashrom -p raiden_debug --flash-name` && `-r`.
Change-Id: Ifad273a708acea4de797a0808be58960635a8864 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M Makefile M flashrom.c M meson.build M meson_options.txt M programmer.h A raiden_debug_spi.c A usb_device.c A usb_device.h 8 files changed, 977 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/09/38209/2
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 2:
Edward, what is 'servo debug tool' - it would be good to describe it here even if briefly.
Hello Angel Pons, Stefan Reinauer, build bot (Jenkins), Vadim Bendebury, Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/38209
to look at the new patch set (#3).
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
raiden_debug: Upstream ChromiumOS servo debug board prog
Initial check-in of the Raiden debugger programmer for the ChromiumOS servo debug tool. This then allows upstream flashrom to fully support the servo debug hardware tool.
Servo is a debug board used for Chromium OS test and development. Depending on the version of Servo, it can connect to a debug header or USB port on the Chrome OS device. Servo is a key enabler for testing:
* Software access to device GPIOs, through hdctools, * Access to EC and CPU UART ports, for convenient debugging, * Reflashing of EC and system firmware (recovery of bricked systems).
For example, it can act as a USB host to simulate connection and removal of external USB devices. It also provides JTAG/SWD support.
The raiden_debug programmer supports:
* Servo v4, * Servo micro, * suzy-q CCD debug cable.
The Servo v2 is actually supported by the ft2232 programmer driver.
While it isn't ideal to have usb_device.[h|c] when usbdev.c already exists in-tree, we get the initial raiden_debug supporting code in so we can massage it moving forwards.
Squash in, usb_device: needed for raiden_debug prog
BUG=b:143389556 BRANCH=none TEST=builds && ran `./flashrom -p raiden_debug --flash-name` && `-r`.
Change-Id: Ifad273a708acea4de797a0808be58960635a8864 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M Makefile M flashrom.c M meson.build M meson_options.txt M programmer.h A raiden_debug_spi.c A usb_device.c A usb_device.h 8 files changed, 977 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/09/38209/3
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 3:
Patch Set 2:
Edward, what is 'servo debug tool' - it would be good to describe it here even if briefly.
Done.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@238 PS3, Line 238: ) (void)
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.c File usb_device.c:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.c@86 PS3, Line 86: void usb_match_value_default Why are all externally visible functions undocumented (here, not in the .h file), but all static functions have comments?
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 3: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return That kind of thing breaks people's internal parsers
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 3:
(5 comments)
I like the USB code and its potential to unify programmer parameters. It belongs into a separate patch, though, to ease review.
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@35 PS3, Line 35: * Software Foundation. I guess one could infer GPLv3 compatibility from the 3-clause above. But IANAL, so I better ask. Can we get this under "GPLv2 and later"? I try to encourage contributors to this because of libflashrom. Having a GPLv2- only library is not of much use these days (won't get better in the future, I guess).
We are still far from a GPLv2+ flashrom core, though :-/
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@154 PS3, Line 154: * maximum USB packet size for the device. NB. Writing and reviewing such comments takes much more time than fixing the interface, IMHO.
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@157 PS3, Line 157: command header maximum size That's kind of true, but highly confusing when one knows what the macros are actually referring to:
JEDEC_BYTE_PROGRAM_OUTSIZE is the size of a full byte-program command not just a header. It's including the byte to be programmed. The calculation is correct nevertheless, because it matches the `header` of a 4BA byte programm.
Anyway, it renders this paragraph useless, IMO.
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return
That kind of thing breaks people's internal parsers
I agree, this macro is a blocker. Hidden control flow statements are not a good idea.
Also, the whole expression is not valid C. Turning a compound statement into an expression ({ ... }) is a GNU extension, IIRC. And with our limited compile testing, I wouldn't want to take any risks.
Also, with an implementation in a C function, one doesn't have to repeat too much boilerplate, e.g.
ret = libusb_control_transfer(...); if (usb_check_error(ret, "OMG, it failed")) return ret;
I think that would be much easier to read than those CHECK(LIBUSB(libusb_... blocks.
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.c File usb_device.c:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.c@35 PS3, Line 35: * Software Foundation. See `raiden_debug_spi.c`.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 3:
(7 comments)
Patch Set 3:
(5 comments)
I like the USB code and its potential to unify programmer parameters. It belongs into a separate patch, though, to ease review.
Ah I squashed it in so this is a functional bit of code upon it's own merits. Maybe we can separate out in the final round? I would like to address all the higher level rubrics required to get a patch of this nature moved from the cros/flashrom tree into the upstream code. license/style/etc as I predicted there was going to be a lot of back and forth to get it into what it needs to be like?
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@35 PS3, Line 35: * Software Foundation.
I guess one could infer GPLv3 compatibility from the 3-clause above. But […]
That is a complex question that I would need to ask legal about? The implementation herein was stripped out of the cros/flashrom fork and isn't just something I wrote vanilla with this header. I'll talk to Stefan for guidance here if you think this blocks getting this merged?
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@154 PS3, Line 154: * maximum USB packet size for the device.
NB. Writing and reviewing such comments takes much more time than fixing the […]
I am not the author and I don't have enough control myself to change all the servo's firmwares to make this perfect from the getgo.. I simply don't think fixing interfaces is attainable in the initial support patch but maybe as a longer term goal.
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@157 PS3, Line 157: command header maximum size
That's kind of true, but highly confusing when one knows what the macros […]
Sorry I wasn't clear on the expectation on what you wish me to change here if anything?
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@238 PS3, Line 238: )
(void)
Done
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return
I agree, this macro is a blocker. Hidden control flow statements are not […]
I brought up the same point in the cros/flashrom tree. I very much agree. Can it be it's own patch though or do I need to fix this first in our tree then rebase this patch from our tree again? It's hard to juggle two trees when my goal is to make one go away in the end.
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.c File usb_device.c:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.c@35 PS3, Line 35: * Software Foundation.
See `raiden_debug_spi.c`.
ditto, see there.
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.c@86 PS3, Line 86: void usb_match_value_default
Why are all externally visible functions undocumented (here, not in the . […]
I am aware, however this is the situation in the cros/flashrom tree. If I can get the functional differences sorted I can address these things as follow ups. However doing them together adds more diff and is too much up front work in one go to make that goal attainable since there is so many cases such as these.
Hello Angel Pons, Stefan Reinauer, David Hendricks, build bot (Jenkins), Nico Huber, Vadim Bendebury, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/38209
to look at the new patch set (#4).
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
raiden_debug: Upstream ChromiumOS servo debug board prog
Initial check-in of the Raiden debugger programmer.
Squash in, usb_device: needed for raiden_debug prog raiden_debug: Add missing .write_aai cb fn
BUG=b:143389556 BRANCH=none TEST=builds
Change-Id: Ifad273a708acea4de797a0808be58960635a8864 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M Makefile M flashrom.c M meson.build M meson_options.txt M programmer.h A raiden_debug_spi.c A usb_device.c A usb_device.h 8 files changed, 978 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/09/38209/4
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@35 PS3, Line 35: * Software Foundation.
That is a complex question that I would need to ask legal about? The implementation herein was strip […]
This code was developed by our team, hence we can re-license it under any license we see fit.
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@154 PS3, Line 154: * maximum USB packet size for the device.
I am not the author and I don't have enough control myself to change all the servo's firmwares to ma […]
Do you have a suggested fix, Nico?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return
I brought up the same point in the cros/flashrom tree. I very much agree. […]
As Nico stated, it is a blocker. Personally, I can't approve a change which introduces it.
Damien Zammit has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
Please accept this change.
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return
As Nico stated, it is a blocker. Personally, I can't approve a change which introduces it.
Edward is doing flashrom community a great service by upstreaming this work. Can we please accommodate that the first check-in will not be perfect? We need to realise that his aim is to remove a whole fork so others can work upstream. As stated, he can submit further patches to fix this small problem, but please don't block new functionality from entering the tree over some minor detail.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 4:
(3 comments)
I like the USB code and its potential to unify programmer parameters. It belongs into a separate patch, though, to ease review.
Ah I squashed it in so this is a functional bit of code upon it's own merits. Maybe we can separate out in the final round?
Final round of what? This review here? Reviewing two patches of 500 lines each usually goes twice as fast as reviewing a big 1k line one... Gerrit is a great tool to assist with reviewing, but that doesn't help if patches aren't organized for review.
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@154 PS3, Line 154: * maximum USB packet size for the device.
Do you have a suggested fix, Nico?
Well, the basic issue seems to be that programmers don't always have max read/write sizes, but a max transfer size instead. I would simply add a field for that. There are only five lines consuming `max_data_(read|write)` in the whole flashrom code, that would need to be adapted.
Maybe make them use helper functions like:
size_t spi_max_read(spi, header_size);
that would interpret the fields in `spi` and subtract `header_size` in case. The tricky part might be to figure out the exact `header_size`, but if we are allowed to make educated guesses here in programmer code, we could as well there.
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@157 PS3, Line 157: command header maximum size
Sorry I wasn't clear on the expectation on what you wish me to change here if anything?
The paragraph is technically wrong. I would drop it or tell the truth. Or get rid of the entire problem (see above).
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return
I brought up the same point in the cros/flashrom tree. I very much agree. Can it be it's own patch though or do I need to fix this first in our tree then rebase this patch from our tree again? It's hard to juggle two trees when my goal is to make one go away in the end.
Edward, if you see issues in the cros tree, please always fix them ahead. It's a huge waste of time for the flashrom community to review bad code, especially when the submitter already knows that it's in bad shape.
About patch juggling. I would simply write new commits for upstreaming, with all known issues already addressed. Once it's merged, push a commit with the resulting diff downstream. If the patches are not in good shape for upstream, it just wastes resources on both sides (mostly of unpaid individuals, but I don't see how Google could gain anything from ugly reviews either).
Edward is doing flashrom community a great service by upstreaming this work.
Damien, can you please explain this claim. Who is this flashrom community? FWIW, it's the exact opposite with the chromium-fork upstreaming: Nobody helps the community with review resources, no help to get better automated build tests running. Instead, most remaining resources are drawn towards the upstreaming, which stalls more urgent flashrom work and delays new releases.
Can we please accommodate that the first check-in will not be perfect? We need to realise that his aim is to remove a whole fork so others can work upstream.
Again, please explain what this opinion is based on. Who are these "others"? Who said that they want to work upstream, which would include helping with the project and not bluntly congesting it. I've seen no sign so far, that things could remotely get better.
As stated, he can submit further patches to fix this small problem, but please don't block new functionality from entering the tree over some minor detail.
Sorry, I've moved past that. Promises of future work have turned into the opposite so far. Plus, Edward has just shown that he won't even fix serious regressions.
And, wtf, potential build breakage is a minor detail now?
NB. I wanted to continue review on this today. Now you are drawing resources too.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 4:
(1 comment)
Patch Set 4:
(3 comments)
I like the USB code and its potential to unify programmer parameters. It belongs into a separate patch, though, to ease review.
Ah I squashed it in so this is a functional bit of code upon it's own merits. Maybe we can separate out in the final round?
Final round of what? This review here? Reviewing two patches of 500 lines each usually goes twice as fast as reviewing a big 1k line one... Gerrit is a great tool to assist with reviewing, but that doesn't help if patches aren't organized for review.
I mean first round because I had to get it to even build, fit and work in the upstream tree. If I prepared the usb patch on its own then that would be dead code and one could quite easily complain that I am asking for review for code that isn't even used yet. It's hard to telescope ahead and see how people will take it so beginning with some self-contained code that does a thing seemed like a reasonable starting point. That was my rational.
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return
I brought up the same point in the cros/flashrom tree. I very much agree. […]
Sorry for the delay on this, there has been holidays here and large fires across Australia..
This is getting fixed in our tree here: https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+...
Nico, that's all well and good saying "just fix everything all at once and everything is good then" but in practice that is extremely hard juggling two trees that have diverged over a decade. Your talking as if I always worked at Google and haven't been an external contributor before using my own personal time reviewing patches. I think that is what Damien is getting at here in that I am well aware of the challenges on both sides.
I think your comment about me not fixing issues is a bit insensitive if you happen to be following the news.. There are real people behind keyboards that face life challenges however I digress.
Regarding resourcing the flashrom project. That is exactly the aim I am pushing for internally is to STOP Google going off and writing their own replacement, instead converge with upstream and provide it resources it needs to live and grow. It is very challenging to actually get to that state simply on my own with every step precision perfect every time. I am looking to work *with the community* here and not looking for criticism. If there is a issue lets fix things together, work together and get this converged so we can actually get to a better place.
TL;DR: I will most certainly fix issues to the best of my ability but I cannot promise every step I make is perfect in bringing together two trees after 9y apart with zero tests.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return
Sorry for the delay on this, there has been holidays here and large fires across Australia.. […]
I have checked the patch in downstream, and it greatly improves readability. Thank you very much!
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return
I have checked the patch in downstream, and it greatly improves readability. […]
Hey Angel,
Thanks very much. I am working through the licensing stuff as well here and as soon as I have the various things addressed brought up here merged in our branch i'll follow up with the revised usb_device.{h,c} and raiden programmer support patchsets. I also noticed this header really should have guards as well so I will fix that locally as well.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return
Hey Angel, […]
https://review.coreboot.org/c/flashrom/+/38936 I have hopefully addressed everything required for the usb_device.[h,c] including clean ups and re-licensing as required for upstreaming.
Hello Angel Pons, Stefan Reinauer, David Hendricks, build bot (Jenkins), Nico Huber, Vadim Bendebury, Damien Zammit, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/38209
to look at the new patch set (#5).
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
raiden_debug: Upstream ChromiumOS servo debug board prog
Initial check-in of the Raiden debugger programmer.
Squash in, raiden_debug: Add missing .write_aai cb fn
BUG=b:143389556 BRANCH=none TEST=builds
Change-Id: Ifad273a708acea4de797a0808be58960635a8864 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M Makefile M flashrom.c M meson.build M meson_options.txt M programmer.h A raiden_debug_spi.c 6 files changed, 432 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/09/38209/5
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 5: Code-Review+2
(2 comments)
https://review.coreboot.org/c/flashrom/+/38209/5/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/38209/5/raiden_debug_spi.c@75 PS5, Line 75: * This timeout is so large because the Raiden SPI timeout is 800ms. Where is the additional 200 coming from?
https://review.coreboot.org/c/flashrom/+/38209/5/raiden_debug_spi.c@94 PS5, Line 94: msg_perr("Raiden: invalid write_count of %d\n", write_count); invalid -> Invalid?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/flashrom/+/38209/5/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/38209/5/raiden_debug_spi.c@75 PS5, Line 75: * This timeout is so large because the Raiden SPI timeout is 800ms.
Where is the additional 200 coming from?
Done. https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+...
https://review.coreboot.org/c/flashrom/+/38209/5/raiden_debug_spi.c@94 PS5, Line 94: msg_perr("Raiden: invalid write_count of %d\n", write_count);
invalid -> Invalid?
Done. https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+...
Hello build bot (Jenkins), Nico Huber, Vadim Bendebury, Damien Zammit, Patrick Georgi, Stefan Reinauer, David Hendricks, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/38209
to look at the new patch set (#6).
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
raiden_debug: Upstream ChromiumOS servo debug board prog
Initial check-in of the Raiden debugger programmer.
Squash in, raiden_debug: Add missing .write_aai cb fn raiden_debug: greatly improve protocol documentation
BUG=b:143389556 BRANCH=none TEST=builds
Change-Id: Ifad273a708acea4de797a0808be58960635a8864 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M Makefile M flashrom.c M meson.build M meson_options.txt M programmer.h A raiden_debug_spi.c 6 files changed, 489 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/09/38209/6
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 6: Code-Review+2
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
raiden_debug: Upstream ChromiumOS servo debug board prog
Initial check-in of the Raiden debugger programmer.
Squash in, raiden_debug: Add missing .write_aai cb fn raiden_debug: greatly improve protocol documentation
BUG=b:143389556 BRANCH=none TEST=builds
Change-Id: Ifad273a708acea4de797a0808be58960635a8864 Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/38209 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Stefan Reinauer stefan.reinauer@coreboot.org --- M Makefile M flashrom.c M meson.build M meson_options.txt M programmer.h A raiden_debug_spi.c 6 files changed, 489 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved
diff --git a/Makefile b/Makefile index bfd4d1e..28ceab6 100644 --- a/Makefile +++ b/Makefile @@ -226,6 +226,11 @@ else override CONFIG_RAYER_SPI = no endif +ifeq ($(CONFIG_RAIDEN), yes) +UNSUPPORTED_FEATURES += CONFIG_RAIDEN=yes +else +override CONFIG_RAIDEN = no +endif ifeq ($(CONFIG_NIC3COM), yes) UNSUPPORTED_FEATURES += CONFIG_NIC3COM=yes else @@ -607,6 +612,9 @@ # RayeR SPIPGM hardware support CONFIG_RAYER_SPI ?= yes
+# ChromiumOS servo DUT debug board hardware support +CONFIG_RAIDEN ?= yes + # PonyProg2000 SPI hardware support CONFIG_PONY_SPI ?= yes
@@ -817,6 +825,11 @@ NEED_RAW_ACCESS += CONFIG_RAYER_SPI endif
+ifeq ($(CONFIG_RAIDEN), yes) +FEATURE_CFLAGS += -D'CONFIG_RAIDEN=1' +PROGRAMMER_OBJS += raiden_debug_spi.o +endif + ifeq ($(CONFIG_PONY_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_PONY_SPI=1' PROGRAMMER_OBJS += pony_spi.o diff --git a/flashrom.c b/flashrom.c index e540027..e541b68 100644 --- a/flashrom.c +++ b/flashrom.c @@ -133,6 +133,18 @@ }, #endif
+#if CONFIG_RAIDEN == 1 + { + .name = "raiden_debug", + .type = USB, + .devs.note = "All programmer devices speaking the raiden protocol\n", + .init = raiden_debug_spi_init, + .map_flash_region = fallback_map, + .unmap_flash_region = fallback_unmap, + .delay = internal_delay, + }, +#endif + #if CONFIG_DRKAISER == 1 { .name = "drkaiser", diff --git a/meson.build b/meson.build index 45a3681..8bb3e65 100644 --- a/meson.build +++ b/meson.build @@ -42,6 +42,7 @@ config_dummy = get_option('config_dummy') config_ft2232_spi = get_option('config_ft2232_spi') config_gfxnvidia = get_option('config_gfxnvidia') +config_raiden = get_option('config_raiden') config_internal = get_option('config_internal') config_it8212 = get_option('config_it8212') config_linux_mtd = get_option('config_linux_mtd') @@ -105,6 +106,7 @@ config_atavia = false config_drkaiser = false config_gfxnvidia = false + config_raiden = false config_internal = false config_it8212 = false config_nic3com = false @@ -171,6 +173,10 @@ srcs += 'gfxnvidia.c' cargs += '-DCONFIG_GFXNVIDIA=1' endif +if config_raiden + srcs += 'raiden_debug_spi.c' + cargs += '-DCONFIG_RAIDEN=1' +endif if config_internal srcs += 'board_enable.c' srcs += 'cbtable.c' diff --git a/meson_options.txt b/meson_options.txt index ea87311..2831271 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -13,6 +13,7 @@ option('config_dummy', type : 'boolean', value : true, description : 'dummy tracing') option('config_ft2232_spi', type : 'boolean', value : true, description : 'FT2232 SPI dongles') option('config_gfxnvidia', type : 'boolean', value : true, description : 'NVIDIA graphics cards') +option('config_raiden', type : 'boolean', value : true, description : 'ChromiumOS Servo DUT debug board') option('config_internal', type : 'boolean', value : true, description : 'internal/onboard') option('config_internal_dmi', type : 'boolean', value : true, description : 'Use internal DMI parser') option('config_it8212', type : 'boolean', value : true, description : 'ITE IT8212F PATA') diff --git a/programmer.h b/programmer.h index 3cf53b9..08500c6 100644 --- a/programmer.h +++ b/programmer.h @@ -43,6 +43,9 @@ #if CONFIG_GFXNVIDIA == 1 PROGRAMMER_GFXNVIDIA, #endif +#if CONFIG_RAIDEN == 1 + PROGRAMMER_RAIDEN, +#endif #if CONFIG_DRKAISER == 1 PROGRAMMER_DRKAISER, #endif @@ -401,6 +404,11 @@ extern const struct dev_entry gfx_nvidia[]; #endif
+/* raiden_debug_spi.c */ +#if CONFIG_RAIDEN == 1 +int raiden_debug_spi_init(void); +#endif + /* drkaiser.c */ #if CONFIG_DRKAISER == 1 int drkaiser_init(void); diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c new file mode 100644 index 0000000..84b99ab --- /dev/null +++ b/raiden_debug_spi.c @@ -0,0 +1,449 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2014, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * Alternatively, this software may be distributed under the terms of the + * GNU General Public License ("GPL") version 2 as published by the Free + * Software Foundation. + */ + +/* + * This SPI flash programming interface is designed to talk to a Chromium OS + * device over a Raiden USB connection. The USB connection is routed to a + * microcontroller running an image compiled from: + * + * https://chromium.googlesource.com/chromiumos/platform/ec + * + * The protocol for the USB-SPI bridge is documented in the following file in + * that respository: + * + * chip/stm32/usb_spi.c + * + * Version 1: + * SPI transactions of up to 62B in each direction with every command having + * a response. The initial packet from host contains a 2B header indicating + * write and read counts with an optional payload length equal to the write + * count. The device will respond with a message that reports the 2B status + * code and an optional payload response length equal to read count. + * + * Message Format: + * + * Command Packet: + * +------------------+-----------------+------------------------+ + * | write count : 1B | read count : 1B | write payload : <= 62B | + * +------------------+-----------------+------------------------+ + * + * write count: 1 byte, zero based count of bytes to write + * + * read count: 1 byte, zero based count of bytes to read + * + * write payload: Up to 62 bytes of data to write to SPI, the total + * length of all TX packets must match write count. + * Due to data alignment constraints, this must be an + * even number of bytes unless this is the final packet. + * + * Response Packet: + * +-------------+-----------------------+ + * | status : 2B | read payload : <= 62B | + * +-------------+-----------------------+ + * + * status: 2 byte status + * 0x0000: Success + * 0x0001: SPI timeout + * 0x0002: Busy, try again + * This can happen if someone else has acquired the shared memory + * buffer that the SPI driver uses as /dev/null + * 0x0003: Write count invalid (V1 > 62B) + * 0x0004: Read count invalid (V1 > 62B) + * 0x0005: The SPI bridge is disabled. + * 0x8000: Unknown error mask + * The bottom 15 bits will contain the bottom 15 bits from the EC + * error code. + * + * read payload: Up to 62 bytes of data read from SPI, the total + * length of all RX packets must match read count + * unless an error status was returned. Due to data + * alignment constraints, this must be a even number + * of bytes unless this is the final packet. + * + * USB Error Codes: + * + * send_command return codes have the following format: + * + * 0x00000: Status code success. + * 0x00001-0x0FFFF: Error code returned by the USB SPI device. + * 0x10001-0x1FFFF: The host has determined an error has occurred. + * 0x20001-0x20063 Lower bits store the positive value representation + * of the libusb_error enum. See the libusb documentation: + * http://libusb.sourceforge.net/api-1.0/group__misc.html + */ + +#include "programmer.h" +#include "spi.h" +#include "usb_device.h" + +#include <libusb.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> + +#define GOOGLE_VID (0x18D1) +#define GOOGLE_RAIDEN_SPI_SUBCLASS (0x51) +#define GOOGLE_RAIDEN_SPI_PROTOCOL (0x01) + + +enum raiden_debug_spi_request { + RAIDEN_DEBUG_SPI_REQ_ENABLE = 0x0000, + RAIDEN_DEBUG_SPI_REQ_DISABLE = 0x0001, + RAIDEN_DEBUG_SPI_REQ_ENABLE_AP = 0x0002, + RAIDEN_DEBUG_SPI_REQ_ENABLE_EC = 0x0003, +}; + +#define PACKET_HEADER_SIZE 2 +#define MAX_PACKET_SIZE 64 + +/* + * This timeout is so large because the Raiden SPI timeout is 800ms. + */ +#define TRANSFER_TIMEOUT_MS (200 + 800) + +struct usb_device *device = NULL; +uint8_t in_endpoint = 0; +uint8_t out_endpoint = 0; + +static int send_command(struct flashctx *flash, + unsigned int write_count, + unsigned int read_count, + const unsigned char *write_buffer, + unsigned char *read_buffer) +{ + uint8_t buffer[MAX_PACKET_SIZE]; + int transferred; + int ret; + + if (write_count > MAX_PACKET_SIZE - PACKET_HEADER_SIZE) { + msg_perr("Raiden: invalid write_count of %d\n", write_count); + return SPI_INVALID_LENGTH; + } + + if (read_count > MAX_PACKET_SIZE - PACKET_HEADER_SIZE) { + msg_perr("Raiden: invalid read_count of %d\n", read_count); + return SPI_INVALID_LENGTH; + } + + buffer[0] = write_count; + buffer[1] = read_count; + + memcpy(buffer + PACKET_HEADER_SIZE, write_buffer, write_count); + + ret = LIBUSB(libusb_bulk_transfer(device->handle, + out_endpoint, + buffer, + write_count + PACKET_HEADER_SIZE, + &transferred, + TRANSFER_TIMEOUT_MS)); + if (ret != 0) { + msg_perr("Raiden: OUT transfer failed\n" + " write_count = %d\n" + " read_count = %d\n", + write_count, read_count); + return ret; + } + + if ((unsigned) transferred != write_count + PACKET_HEADER_SIZE) { + msg_perr("Raiden: Write failure (wrote %d, expected %d)\n", + transferred, write_count + PACKET_HEADER_SIZE); + return 0x10001; + } + + ret = LIBUSB(libusb_bulk_transfer(device->handle, + in_endpoint, + buffer, + read_count + PACKET_HEADER_SIZE, + &transferred, + TRANSFER_TIMEOUT_MS)); + if (ret != 0) { + msg_perr("Raiden: IN transfer failed\n" + " write_count = %d\n" + " read_count = %d\n", + write_count, read_count); + return ret; + } + + if ((unsigned) transferred != read_count + PACKET_HEADER_SIZE) { + msg_perr("Raiden: Read failure (read %d, expected %d)\n", + transferred, read_count + PACKET_HEADER_SIZE); + return 0x10002; + } + + memcpy(read_buffer, buffer + PACKET_HEADER_SIZE, read_count); + + return buffer[0] | (buffer[1] << 8); +} + +/* + * Unfortunately there doesn't seem to be a way to specify the maximum number + * of bytes that your SPI device can read/write, these values are the maximum + * data chunk size that flashrom will package up with an additional five bytes + * of command for the flash device, resulting in a 62 byte packet, that we then + * add two bytes to in either direction, making our way up to the 64 byte + * maximum USB packet size for the device. + * + * The largest command that flashrom generates is the byte program command, so + * we use that command header maximum size here. + */ +#define MAX_DATA_SIZE (MAX_PACKET_SIZE - \ + PACKET_HEADER_SIZE - \ + JEDEC_BYTE_PROGRAM_OUTSIZE) + +static const struct spi_master spi_master_raiden_debug = { + .features = SPI_MASTER_4BA, + .max_data_read = MAX_DATA_SIZE, + .max_data_write = MAX_DATA_SIZE, + .command = send_command, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, + .write_aai = default_spi_write_aai, +}; + +static int match_endpoint(struct libusb_endpoint_descriptor const *descriptor, + enum libusb_endpoint_direction direction) +{ + return (((descriptor->bEndpointAddress & LIBUSB_ENDPOINT_DIR_MASK) == + direction) && + ((descriptor->bmAttributes & LIBUSB_TRANSFER_TYPE_MASK) == + LIBUSB_TRANSFER_TYPE_BULK)); +} + +static int find_endpoints(struct usb_device *dev, uint8_t *in_ep, uint8_t *out_ep) +{ + int i; + int in_count = 0; + int out_count = 0; + + for (i = 0; i < dev->interface_descriptor->bNumEndpoints; i++) { + struct libusb_endpoint_descriptor const *endpoint = + &dev->interface_descriptor->endpoint[i]; + + if (match_endpoint(endpoint, LIBUSB_ENDPOINT_IN)) { + in_count++; + *in_ep = endpoint->bEndpointAddress; + } else if (match_endpoint(endpoint, LIBUSB_ENDPOINT_OUT)) { + out_count++; + *out_ep = endpoint->bEndpointAddress; + } + } + + if (in_count != 1 || out_count != 1) { + msg_perr("Raiden: Failed to find one IN and one OUT endpoint\n" + " found %d IN and %d OUT endpoints\n", + in_count, + out_count); + return 1; + } + + msg_pdbg("Raiden: Found IN endpoint = 0x%02x\n", *in_ep); + msg_pdbg("Raiden: Found OUT endpoint = 0x%02x\n", *out_ep); + + return 0; +} + +static int shutdown(void * data) +{ + int ret = LIBUSB(libusb_control_transfer( + device->handle, + LIBUSB_ENDPOINT_OUT | + LIBUSB_REQUEST_TYPE_VENDOR | + LIBUSB_RECIPIENT_INTERFACE, + RAIDEN_DEBUG_SPI_REQ_DISABLE, + 0, + device->interface_descriptor->bInterfaceNumber, + NULL, + 0, + TRANSFER_TIMEOUT_MS)); + if (ret != 0) { + msg_perr("Raiden: Failed to disable SPI bridge\n"); + return ret; + } + + usb_device_free(device); + + device = NULL; + libusb_exit(NULL); + + return 0; +} + +static int get_target(void) +{ + int request_enable = RAIDEN_DEBUG_SPI_REQ_ENABLE; + + char *target_str = extract_programmer_param("target"); + if (target_str) { + if (!strcasecmp(target_str, "ap")) + request_enable = RAIDEN_DEBUG_SPI_REQ_ENABLE_AP; + else if (!strcasecmp(target_str, "ec")) + request_enable = RAIDEN_DEBUG_SPI_REQ_ENABLE_EC; + else { + msg_perr("Invalid target: %s\n", target_str); + request_enable = -1; + } + } + free(target_str); + + return request_enable; +} + +static void free_dev_list(struct usb_device **dev_lst) +{ + struct usb_device *dev = *dev_lst; + /* free devices we don't care about */ + dev = dev->next; + while (dev) + dev = usb_device_free(dev); +} + +int raiden_debug_spi_init(void) +{ + struct usb_match match; + char *serial = extract_programmer_param("serial"); + struct usb_device *current; + int found = 0; + int ret; + + int request_enable = get_target(); + if (request_enable < 0) + return 1; + + usb_match_init(&match); + + usb_match_value_default(&match.vid, GOOGLE_VID); + usb_match_value_default(&match.class, LIBUSB_CLASS_VENDOR_SPEC); + usb_match_value_default(&match.subclass, GOOGLE_RAIDEN_SPI_SUBCLASS); + usb_match_value_default(&match.protocol, GOOGLE_RAIDEN_SPI_PROTOCOL); + + ret = LIBUSB(libusb_init(NULL)); + if (ret != 0) { + msg_perr("Raiden: libusb_init failed\n"); + return ret; + } + + ret = usb_device_find(&match, ¤t); + if (ret != 0) { + msg_perr("Raiden: Failed to find devices\n"); + return ret; + } + + while (current) { + device = current; + + if (find_endpoints(device, &in_endpoint, &out_endpoint)) { + msg_pdbg("Raiden: Failed to find valid endpoints on device"); + usb_device_show(" ", current); + goto loop_end; + } + + if (usb_device_claim(device)) { + msg_pdbg("Raiden: Failed to claim USB device"); + usb_device_show(" ", current); + goto loop_end; + } + + if (!serial) { + found = 1; + goto loop_end; + } else { + unsigned char dev_serial[32]; + struct libusb_device_descriptor descriptor; + int rc; + + memset(dev_serial, 0, sizeof(dev_serial)); + + if (libusb_get_device_descriptor(device->device, &descriptor)) { + msg_pdbg("USB: Failed to get device descriptor.\n"); + goto loop_end; + } + + rc = libusb_get_string_descriptor_ascii(device->handle, + descriptor.iSerialNumber, + dev_serial, + sizeof(dev_serial)); + if (rc < 0) { + LIBUSB(rc); + } else { + if (strcmp(serial, (char *)dev_serial)) { + msg_pdbg("Raiden: Serial number %s did not match device", serial); + usb_device_show(" ", current); + } else { + msg_pinfo("Raiden: Serial number %s matched device", serial); + usb_device_show(" ", current); + found = 1; + } + } + } + +loop_end: + if (found) + break; + else + current = usb_device_free(current); + } + + if (!device || !found) { + msg_perr("Raiden: No usable device found.\n"); + return 1; + } + + free_dev_list(¤t); + + ret = LIBUSB(libusb_control_transfer( + device->handle, + LIBUSB_ENDPOINT_OUT | + LIBUSB_REQUEST_TYPE_VENDOR | + LIBUSB_RECIPIENT_INTERFACE, + request_enable, + 0, + device->interface_descriptor->bInterfaceNumber, + NULL, + 0, + TRANSFER_TIMEOUT_MS)); + if (ret != 0) { + msg_perr("Raiden: Failed to enable SPI bridge\n"); + return ret; + } + + register_spi_master(&spi_master_raiden_debug); + register_shutdown(shutdown, NULL); + + return 0; +}
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 7:
now, make CONFIG_ENABLE_LIBUSB1_PROGRAMMERS=no will fail
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 7:
Patch Set 7:
now, make CONFIG_ENABLE_LIBUSB1_PROGRAMMERS=no will fail
That is because the programmer was not guarded properly, I guess.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
now, make CONFIG_ENABLE_LIBUSB1_PROGRAMMERS=no will fail
That is because the programmer was not guarded properly, I guess.
Thanks for spotting this! https://review.coreboot.org/c/flashrom/+/39896
Has there been any recent discussion on perhaps EOL'ing the Makefile and just having meson?
Attention is currently required from: Edward O'Callaghan. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 7:
(2 comments)
File Makefile:
https://review.coreboot.org/c/flashrom/+/38209/comment/ae2bf0bc_49007891 PS7, Line 229: ifeq ($(CONFIG_RAIDEN), yes) : UNSUPPORTED_FEATURES += CONFIG_RAIDEN=yes : else : override CONFIG_RAIDEN = no : endif What's the reasoning here? Does it need more than libusb?
File meson.build:
https://review.coreboot.org/c/flashrom/+/38209/comment/602752d0_fb165318 PS7, Line 109: config_raiden = false Huh, this dependency is not mentioned in the Makefile...
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 7:
(2 comments)
File Makefile:
https://review.coreboot.org/c/flashrom/+/38209/comment/1639fed3_542d2235 PS7, Line 229: ifeq ($(CONFIG_RAIDEN), yes) : UNSUPPORTED_FEATURES += CONFIG_RAIDEN=yes : else : override CONFIG_RAIDEN = no : endif
What's the reasoning here? Does it need more than libusb?
No it shouldn't. I can't fully recall now however I *think* this ended up here because it was never tested on Windows or maybe I just got lost in all the ifdef's.
I have prepared a patch to drop the hunk: https://review.coreboot.org/c/flashrom/+/51930
File meson.build:
https://review.coreboot.org/c/flashrom/+/38209/comment/c7dc72c1_8cfad78b PS7, Line 109: config_raiden = false
Huh, this dependency is not mentioned in the Makefile...
this is most definitely incorrect! thanks, the line somehow founds its way from the above block, fixed in the above mentioned patch.
-
Angel Pons (Code Review) -
Damien Zammit (Code Review)
-
Edward O'Callaghan (Code Review)
-
HAOUAS Elyes (Code Review)
-
Nico Huber (Code Review)
-
Stefan Reinauer (Code Review)
-
Vadim Bendebury (Code Review)