Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/77999?usp=email )
Change subject: raiden: Support target index with generic REQ_ENABLE ......................................................................
raiden: Support target index with generic REQ_ENABLE
Some devices such as the GSC knows how it is wired to AP and EC flash chips, and can be told which specific device to talk to. Other devices such as Servo Micro and HyperDebug are generic, and do not know how they are wired, the caller is responsible for first configure the appropriate MUXes or buffers, and then tell the debugger which port to use (Servo Micro has just one SPI port, HyperDebug is the first that has multiple). The Raiden protocol allows both the cases of USB devices knowing their wiring and not.
If I were to declare the protocol in Rust, this is how the information of the Raiden protocol "enable request" would be encoded: ``` enum { EnableGeneric(u8), EnableAp, EnableEc, ... } ```
The first label `EnableGeneric(u8)` is to be used with HyperDebug that does not know how its ports are wired, and allow access by index. The other labels `EnableAp` and `EnableEc` are to be used with the GSC.
The actual transmission of the enum above uses the bRequest and low byte of wValue of a USB control request, but that is a detail and not conceptually important.
Until now, `-p raiden_debug_spi:target=AP` or `...:target=EC` could be used to make flashrom use `EnableAp` or `EnableEc`, and if neither was given, it would default to `EnableGeneric`, which now that wValue is used means `EnableGeneric(0)`.
I find it rather straight-forward, that `-p raiden_debug_spi:target=1`, `...:target=2`, etc. should translate to `EnableGeneric(1)`, etc.
This patchset achieves this, by adding a second 16-bit parameter value, next to request_enable.
I have tested that flashrom can detect the same Winbond flash chip "W25Q128.V..M" with two different Raiden USB devices as below.
TEST=flashrom -p raiden_debug_spi:serial=0701B044-91AC3132,target=AP TEST=flashrom -p raiden_debug_spi:serial=205635783236,target=1
Signed-off-by: Jes B. Klinke jbk@chromium.org Change-Id: I03bf4f3210186fb5937b42e298761907b03e08b7 Reviewed-on: https://review.coreboot.org/c/flashrom/+/77999 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Anastasia Klimchuk aklm@chromium.org --- M doc/classic_cli_manpage.rst M raiden_debug_spi.c M tests/raiden_debug_spi.c M tests/tests.c M tests/tests.h 5 files changed, 132 insertions(+), 19 deletions(-)
Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved
diff --git a/doc/classic_cli_manpage.rst b/doc/classic_cli_manpage.rst index 9a455b0..5d6b7ef 100644 --- a/doc/classic_cli_manpage.rst +++ b/doc/classic_cli_manpage.rst @@ -865,11 +865,15 @@ raiden_debug_spi programmer ^^^^^^^^^^^^^^^^^^^^^^^^^^^
-The target of the SPI flashing mux must be specified with the ``target`` parameter with the:: +Some devices such as the GSC knows how it is wired to AP and EC flash chips, and can be told which specific device to talk to using the ``target`` parameter::
- flashrom -p raiden_debug_spi:target=chip + flashrom -p raiden_debug_spi:target={ap,ec}
-syntax, where ``chip`` is either the ``ap`` or ``ec`` to flash, otherwise a unspecified target terminates at the end-point. +Other devices such as Servo Micro and HyperDebug are generic, and do not know how they are wired, the caller is responsible for first configure the appropriate MUXes or buffers, and then tell the debugger which port to use (Servo Micro has just one SPI port, HyperDebug is the first of this kind to have multiple):: + + flashrom -p raiden_debug_spi:target=N + +where ``N`` is an non-negative integer (default ``0``).
The default is to use the first available servo. You can use the optional ``serial`` parameter to specify the servo USB device serial number to use specifically with:: diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index c5642ff..8fd5f74 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -1450,29 +1450,43 @@ return ap_request; }
-static int get_target(const struct programmer_cfg *cfg) +static int decode_programmer_param(const struct programmer_cfg *cfg, uint8_t *request, + uint16_t *request_parameter) { /** * REQ_ENABLE doesn't specify a target bus, and will be rejected * by adapters that support more than one target. */ - int request_enable = RAIDEN_DEBUG_SPI_REQ_ENABLE; + uint8_t request_enable = RAIDEN_DEBUG_SPI_REQ_ENABLE; + uint16_t parameter = 0; + int ret = 0;
char *target_str = extract_programmer_param_str(cfg, "target"); + printf("FISK: %s\n", target_str); + if (target_str) { - if (!strcasecmp(target_str, "ap")) + char *endptr; + int index = strtol(target_str, &endptr, 0); + if (*target_str && !*endptr && index >= 0 && index < 256) { + request_enable = RAIDEN_DEBUG_SPI_REQ_ENABLE; + parameter = index; + } else if (!strcasecmp(target_str, "ap")) request_enable = get_ap_request_type(cfg); 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; + ret = 1; } } free(target_str); - msg_pinfo("Raiden target: %d\n", request_enable); + if (ret == 0) { + msg_pinfo("Raiden target: %d,%d\n", request_enable, parameter);
- return request_enable; + *request = request_enable; + *request_parameter = parameter; + } + return ret; }
static void free_dev_list(struct usb_device **dev_lst) @@ -1493,10 +1507,12 @@ bool found = false; int ret;
- int request_enable = get_target(cfg); - if (request_enable < 0) { + uint8_t request_enable; + uint16_t request_parameter; + ret = decode_programmer_param(cfg, &request_enable, &request_parameter); + if (ret != 0) { free(serial); - return 1; + return ret; }
usb_match_init(cfg, &match); @@ -1588,7 +1604,7 @@ LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_INTERFACE, request_enable, - 0, + request_parameter, device->interface_descriptor->bInterfaceNumber, NULL, 0, diff --git a/tests/raiden_debug_spi.c b/tests/raiden_debug_spi.c index 5c79a90..4f0bdd3 100644 --- a/tests/raiden_debug_spi.c +++ b/tests/raiden_debug_spi.c @@ -104,15 +104,100 @@ .fallback_open_state = &raiden_debug_fallback_open_state, };
- /* - * 12 is the length of programmer param string for 3-digit address. - * Address can be max 3-digit because it needs to fit into uint8_t. - */ - char raiden_debug_param[12]; - snprintf(raiden_debug_param, 12, "address=%d", USB_DEVICE_ADDRESS); + char raiden_debug_param[32];
+ snprintf(raiden_debug_param, sizeof(raiden_debug_param), + "address=%d", USB_DEVICE_ADDRESS); + run_basic_lifecycle(state, &raiden_debug_io, &programmer_raiden_debug_spi, raiden_debug_param); +} + +void raiden_debug_targetAP_basic_lifecycle_test_success(void **state) +{ + struct io_mock_fallback_open_state raiden_debug_fallback_open_state = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock raiden_debug_io = { + .libusb_get_device_list = raiden_debug_libusb_get_device_list, + .libusb_free_device_list = raiden_debug_libusb_free_device_list, + .libusb_get_device_descriptor = raiden_debug_libusb_get_device_descriptor, + .libusb_get_config_descriptor = raiden_debug_libusb_get_config_descriptor, + .libusb_free_config_descriptor = raiden_debug_libusb_free_config_descriptor, + .fallback_open_state = &raiden_debug_fallback_open_state, + }; + + char raiden_debug_param[32]; + snprintf(raiden_debug_param, sizeof(raiden_debug_param), + "address=%d,target=AP", USB_DEVICE_ADDRESS); + run_basic_lifecycle(state, &raiden_debug_io, &programmer_raiden_debug_spi, raiden_debug_param); +} + +void raiden_debug_targetEC_basic_lifecycle_test_success(void **state) +{ + struct io_mock_fallback_open_state raiden_debug_fallback_open_state = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock raiden_debug_io = { + .libusb_get_device_list = raiden_debug_libusb_get_device_list, + .libusb_free_device_list = raiden_debug_libusb_free_device_list, + .libusb_get_device_descriptor = raiden_debug_libusb_get_device_descriptor, + .libusb_get_config_descriptor = raiden_debug_libusb_get_config_descriptor, + .libusb_free_config_descriptor = raiden_debug_libusb_free_config_descriptor, + .fallback_open_state = &raiden_debug_fallback_open_state, + }; + + char raiden_debug_param[32]; + snprintf(raiden_debug_param, sizeof(raiden_debug_param), + "address=%d,target=ec", USB_DEVICE_ADDRESS); + run_basic_lifecycle(state, &raiden_debug_io, &programmer_raiden_debug_spi, raiden_debug_param); +} + +void raiden_debug_target0_basic_lifecycle_test_success(void **state) +{ + struct io_mock_fallback_open_state raiden_debug_fallback_open_state = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock raiden_debug_io = { + .libusb_get_device_list = raiden_debug_libusb_get_device_list, + .libusb_free_device_list = raiden_debug_libusb_free_device_list, + .libusb_get_device_descriptor = raiden_debug_libusb_get_device_descriptor, + .libusb_get_config_descriptor = raiden_debug_libusb_get_config_descriptor, + .libusb_free_config_descriptor = raiden_debug_libusb_free_config_descriptor, + .fallback_open_state = &raiden_debug_fallback_open_state, + }; + + char raiden_debug_param[32]; + snprintf(raiden_debug_param, sizeof(raiden_debug_param), + "address=%d,target=0", USB_DEVICE_ADDRESS); + run_basic_lifecycle(state, &raiden_debug_io, &programmer_raiden_debug_spi, raiden_debug_param); +} + +void raiden_debug_target1_basic_lifecycle_test_success(void **state) +{ + struct io_mock_fallback_open_state raiden_debug_fallback_open_state = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock raiden_debug_io = { + .libusb_get_device_list = raiden_debug_libusb_get_device_list, + .libusb_free_device_list = raiden_debug_libusb_free_device_list, + .libusb_get_device_descriptor = raiden_debug_libusb_get_device_descriptor, + .libusb_get_config_descriptor = raiden_debug_libusb_get_config_descriptor, + .libusb_free_config_descriptor = raiden_debug_libusb_free_config_descriptor, + .fallback_open_state = &raiden_debug_fallback_open_state, + }; + + char raiden_debug_param[32]; + snprintf(raiden_debug_param, sizeof(raiden_debug_param), + "address=%d,target=1", USB_DEVICE_ADDRESS); run_basic_lifecycle(state, &raiden_debug_io, &programmer_raiden_debug_spi, raiden_debug_param); } #else SKIP_TEST(raiden_debug_basic_lifecycle_test_success) + SKIP_TEST(raiden_debug_targetAP_basic_lifecycle_test_success) + SKIP_TEST(raiden_debug_targetEC_basic_lifecycle_test_success) + SKIP_TEST(raiden_debug_target0_basic_lifecycle_test_success) + SKIP_TEST(raiden_debug_target1_basic_lifecycle_test_success) #endif /* CONFIG_RAIDEN_DEBUG_SPI */ diff --git a/tests/tests.c b/tests/tests.c index 8b4ad03..35bd599 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -463,6 +463,10 @@ cmocka_unit_test(dummy_all_buses_test_success), cmocka_unit_test(nicrealtek_basic_lifecycle_test_success), cmocka_unit_test(raiden_debug_basic_lifecycle_test_success), + cmocka_unit_test(raiden_debug_targetAP_basic_lifecycle_test_success), + cmocka_unit_test(raiden_debug_targetEC_basic_lifecycle_test_success), + cmocka_unit_test(raiden_debug_target0_basic_lifecycle_test_success), + cmocka_unit_test(raiden_debug_target1_basic_lifecycle_test_success), cmocka_unit_test(dediprog_basic_lifecycle_test_success), cmocka_unit_test(linux_mtd_probe_lifecycle_test_success), cmocka_unit_test(linux_spi_probe_lifecycle_test_success), diff --git a/tests/tests.h b/tests/tests.h index e273e1d..3841d20 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -54,6 +54,10 @@ void dummy_all_buses_test_success(void **state); void nicrealtek_basic_lifecycle_test_success(void **state); void raiden_debug_basic_lifecycle_test_success(void **state); +void raiden_debug_targetAP_basic_lifecycle_test_success(void **state); +void raiden_debug_targetEC_basic_lifecycle_test_success(void **state); +void raiden_debug_target0_basic_lifecycle_test_success(void **state); +void raiden_debug_target1_basic_lifecycle_test_success(void **state); void dediprog_basic_lifecycle_test_success(void **state); void linux_mtd_probe_lifecycle_test_success(void **state); void linux_spi_probe_lifecycle_test_success(void **state);
-
Anastasia Klimchuk (Code Review)