May 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: nicintel_eeprom.c: Mark 8086:1531 as tested
by Anastasia Klimchuk (Code Review) 02 May '21

02 May '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Arthur Heymans. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52825 ) Change subject: nicintel_eeprom.c: Mark 8086:1531 as tested ...................................................................... Patch Set 1: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52825/comment/33534280_8587e226 PS1, Line 9: Reading, erasing and writing works on an i210 NIC of an Asus Z10PA-D8. Is it important to have TEST=something something line in commit message? I noticed that some commits have it, some don't. -- To view, visit https://review.coreboot.org/c/flashrom/+/52825 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9cabea5dfb9424b9c30d82840089506f2bd943da Gerrit-Change-Number: 52825 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Sun, 02 May 2021 23:36:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Implement spi_send_multicommand()
by Nico Huber (Code Review) 02 May '21

02 May '21

Attention is currently required from: Simon Buhrow, Edward O'Callaghan, Samir Ibradžić. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Implement spi_send_multicommand() ...................................................................... Patch Set 32: (1 comment) Patchset: PS32: > Not sure if I'm doing something wrong. The patch works so far, but […] I wondered what device was used for testing so far and stumbled over this picture[1] mentioned in the original commit message. There's some 1.36MHz shown in it, so I tried again with such a low speed (divisor=44) and, tada, it makes a difference. No idea what is going on inside that chip. My numbers[2] even suggest that sending the commands in two packets (original chunksize) is faster *in my setup*. [1] https://ibb.co/0c1J25d [2] https://paste.flashrom.org/view.php?id=3472 NB. What would really increase speed in the tested scenario (writing random data, hence everything needs to be erased) is to choose a bigger erase block size. I have this on my todo list since my very first days with flashrom. If I drop 4K and 32K blocks from `flashchips.c`, it's about twice as fast. But as this is not a common use case, I did not see it as a priority. OTOH, it always bugs me, for instance when I switch back and forth between coreboot and another firmware. -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 32 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 02 May 2021 18:26:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Implement spi_send_multicommand()
by Nico Huber (Code Review) 02 May '21

02 May '21

Attention is currently required from: Simon Buhrow, Edward O'Callaghan, Samir Ibradžić. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Implement spi_send_multicommand() ...................................................................... Patch Set 32: (1 comment) Patchset: PS32: Not sure if I'm doing something wrong. The patch works so far, but I don't see any speed increase in my setup. This is with an FT2232H, bcdDevice 7.00 (not sure if that means something) according to lsusb. Running it at 15MHz (divisor=4) with a W25Q128FV. Read/erase/write with random data takes about 11:30min. Maybe it's about 1s faster with these patches, but that could be just noise. I did most tests on a 256KiB layout region, and there was no visible difference ~12.3s. -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 32 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 02 May 2021 17:56:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: lspcon, realtek_mst: move i2c bus resolution from params to i2c_helper
by Angel Pons (Code Review) 02 May '21

02 May '21

Attention is currently required from: Sam McNally, Peter Marheine. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52405 ) Change subject: lspcon, realtek_mst: move i2c bus resolution from params to i2c_helper ...................................................................... Patch Set 9: (1 comment) File i2c_helper_linux.c: https://review.coreboot.org/c/flashrom/+/52405/comment/56ae4840_bf1c55b0 PS6, Line 42: char *dev = malloc(sizeof(I2C_DEV_PREFIX)); > Have you had any interesting ideas? […] I made CB:52830 and CB:52831 to achieve this -- To view, visit https://review.coreboot.org/c/flashrom/+/52405 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieea57b2445f3aca16c87d8f839dff131009531a0 Gerrit-Change-Number: 52405 Gerrit-PatchSet: 9 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Sun, 02 May 2021 17:09:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: realtek_mst_i2c_spi: Use `i2c_open_from_programmer_params`
by Angel Pons (Code Review) 02 May '21

02 May '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52831 ) Change subject: realtek_mst_i2c_spi: Use `i2c_open_from_programmer_params` ...................................................................... realtek_mst_i2c_spi: Use `i2c_open_from_programmer_params` This allows using `buspath` to specify which I2C device to use. Change-Id: Ibdf07a9fde0ddfcda1c0bfa35a3e7cde5c22cedb Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M realtek_mst_i2c_spi.c 1 file changed, 5 insertions(+), 36 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/52831/1 diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 27bcb4a..64579e1 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -442,38 +442,11 @@ return ret; } -static int get_params(int *i2c_bus, int *reset, int *enter_isp) +static int get_params(int *reset, int *enter_isp) { - char *bus_str = NULL, *reset_str = NULL, *isp_str = NULL; + char *reset_str = NULL, *isp_str = NULL; int ret = SPI_GENERIC_ERROR; - bus_str = extract_programmer_param("bus"); - if (bus_str) { - char *bus_suffix; - errno = 0; - int bus = (int)strtol(bus_str, &bus_suffix, 10); - if (errno != 0 || bus_str == bus_suffix) { - msg_perr("%s: Could not convert 'bus'.\n", __func__); - goto _get_params_failed; - } - - if (bus < 0 || bus > 255) { - msg_perr("%s: Value for 'bus' is out of range(0-255).\n", __func__); - goto _get_params_failed; - } - - if (strlen(bus_suffix) > 0) { - msg_perr("%s: Garbage following 'bus' value.\n", __func__); - goto _get_params_failed; - } - - msg_pinfo("Using i2c bus %i.\n", bus); - *i2c_bus = bus; - ret = 0; - } else { - msg_perr("%s: Bus number not specified.\n", __func__); - } - reset_str = extract_programmer_param("reset-mcu"); if (reset_str) { if (reset_str[0] == '1') { @@ -504,22 +477,18 @@ } free(isp_str); -_get_params_failed: - if (bus_str) - free(bus_str); - return ret; } int realtek_mst_i2c_spi_init(void) { int ret = 0; - int i2c_bus = 0, reset = 0, enter_isp = 0; + int reset = 0, enter_isp = 0; - if (get_params(&i2c_bus, &reset, &enter_isp)) + if (get_params(&reset, &enter_isp)) return SPI_GENERIC_ERROR; - int fd = i2c_open(i2c_bus, REGISTER_ADDRESS, 0); + int fd = i2c_open_from_programmer_params(REGISTER_ADDRESS, 0); if (fd < 0) return fd; -- To view, visit https://review.coreboot.org/c/flashrom/+/52831 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibdf07a9fde0ddfcda1c0bfa35a3e7cde5c22cedb Gerrit-Change-Number: 52831 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: lspcon_i2c_spi: Release I2C fd on failed init
by Angel Pons (Code Review) 02 May '21

02 May '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52829 ) Change subject: lspcon_i2c_spi: Release I2C fd on failed init ...................................................................... lspcon_i2c_spi: Release I2C fd on failed init Change-Id: Idec3cd349ab8d6e2ebb0fafae70c5d69bb2c8880 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M lspcon_i2c_spi.c 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/29/52829/1 diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c index b79e7f7..e2c4a3c 100644 --- a/lspcon_i2c_spi.c +++ b/lspcon_i2c_spi.c @@ -502,12 +502,14 @@ int ret = lspcon_i2c_spi_reset_mpu_stop(fd); if (ret) { msg_perr("%s: call to reset_mpu_stop failed.\n", __func__); + i2c_close(fd); return ret; } struct lspcon_i2c_spi_data *data = calloc(1, sizeof(struct lspcon_i2c_spi_data)); if (!data) { msg_perr("Unable to allocate space for extra SPI master data.\n"); + i2c_close(fd); return SPI_GENERIC_ERROR; } -- To view, visit https://review.coreboot.org/c/flashrom/+/52829 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idec3cd349ab8d6e2ebb0fafae70c5d69bb2c8880 Gerrit-Change-Number: 52829 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: lspcon_i2c_spi: Extract I2C bus parameter handling
by Angel Pons (Code Review) 02 May '21

02 May '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52830 ) Change subject: lspcon_i2c_spi: Extract I2C bus parameter handling ...................................................................... lspcon_i2c_spi: Extract I2C bus parameter handling Introduce the `i2c_open_from_programmer_params` function to avoid having to duplicate parameter parsing code on all I2C programmers. This also allows having the same programmer parameters on all I2C programmers. Change-Id: I006b311c88feea37fe4b217f769b21ca1505def9 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M i2c_helper.h M i2c_helper_linux.c M lspcon_i2c_spi.c 3 files changed, 58 insertions(+), 61 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/52830/1 diff --git a/i2c_helper.h b/i2c_helper.h index cab2ce6..709a748 100644 --- a/i2c_helper.h +++ b/i2c_helper.h @@ -80,6 +80,15 @@ int i2c_open_path(const char *path, uint16_t addr, int force); /** + * i2c_open_from_programmer_params: open an I2C device from programmer params + * + * This function is a wrapper for i2c_open and i2c_open_path that obtains the + * I2C device to use from programmer parameters. It is meant to be called + * from I2C-based programmers to avoid repeating parameter parsing code. + */ +int i2c_open_from_programmer_params(uint16_t addr, int force); + +/** * i2c_close - closes the file descriptor returned by i2c_open * * @fd: file descriptor to be closed. diff --git a/i2c_helper_linux.c b/i2c_helper_linux.c index 963c399..1c0577a 100644 --- a/i2c_helper_linux.c +++ b/i2c_helper_linux.c @@ -27,6 +27,7 @@ #include "flash.h" #include "i2c_helper.h" +#include "programmer.h" /* Null characters are placeholders for bus number digits */ #define I2C_DEV_PREFIX "/dev/i2c-\0\0\0" @@ -76,6 +77,53 @@ return i2c_open_path(dev, addr, force); } +/* TODO: Move this function to a common place, it is not specific to I2C */ +static int get_bus_number(char *bus_str) +{ + char *bus_suffix; + errno = 0; + int bus = (int)strtol(bus_str, &bus_suffix, 10); + if (errno != 0 || bus_str == bus_suffix) { + msg_perr("%s: Could not convert 'bus'.\n", __func__); + return -1; + } + + if (strlen(bus_suffix) > 0) { + msg_perr("%s: Garbage following 'bus' value.\n", __func__); + return -1; + } + + msg_pinfo("Using I2C bus %d.\n", bus); + return bus; +} + +int i2c_open_from_programmer_params(uint16_t addr, int force) +{ + int fd = -1; + + char *bus_str = extract_programmer_param("bus"); + char *device_path = extract_programmer_param("devpath"); + + if (device_path != NULL && bus_str != NULL) { + msg_perr("%s: only one of bus and devpath may be specified\n", __func__); + goto out; + } + if (device_path == NULL && bus_str == NULL) { + msg_perr("%s: one of bus and devpath must be specified\n", __func__); + goto out; + } + + if (device_path != NULL) + fd = i2c_open_path(device_path, addr, force); + else + fd = i2c_open(get_bus_number(bus_str), addr, force); + +out: + free(bus_str); + free(device_path); + return fd; +} + int i2c_read(int fd, uint16_t addr, i2c_buffer_t *buf) { if (buf->len == 0) diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c index e2c4a3c..096fb76 100644 --- a/lspcon_i2c_spi.c +++ b/lspcon_i2c_spi.c @@ -433,69 +433,9 @@ return ret; } -/* TODO: remove this out of the specific SPI master implementation. */ -static int get_bus_number(void) -{ - char *bus_str = extract_programmer_param("bus"); - /* Return INVALID_ADDRESS if bus value was given but invalid, and GENERIC_ERROR - * if no value was provided. */ - int ret = SPI_INVALID_ADDRESS; - - if (bus_str == NULL) - return SPI_GENERIC_ERROR; - - char *bus_suffix; - errno = 0; - int bus = (int)strtol(bus_str, &bus_suffix, 10); - if (errno != 0 || bus_str == bus_suffix) { - msg_perr("%s: Could not convert 'bus'.\n", __func__); - goto get_bus_done; - } - - if (bus < 0 || bus > 255) { - msg_perr("%s: Value for 'bus' is out of range(0-255).\n", __func__); - goto get_bus_done; - } - - if (strlen(bus_suffix) > 0) { - msg_perr("%s: Garbage following 'bus' value.\n", __func__); - goto get_bus_done; - } - - msg_pinfo("Using i2c bus %i.\n", bus); - ret = bus; - -get_bus_done: - free(bus_str); - return ret; -} - int lspcon_i2c_spi_init(void) { - int fd = -1; - int bus_number = get_bus_number(); - if (bus_number == SPI_INVALID_ADDRESS) { - /* Bus was specified but unusable, bail out immediately */ - return SPI_GENERIC_ERROR; - } - char *device_path = extract_programmer_param("devpath"); - - if (device_path != NULL && bus_number >= 0) { - msg_perr("%s: only one of bus and devpath may be specified\n", __func__); - free(device_path); - return SPI_GENERIC_ERROR; - } else if (device_path == NULL && bus_number < 0) { - msg_perr("%s: one of bus and devpath must be specified\n", __func__); - return SPI_GENERIC_ERROR; - } - - if (device_path != NULL) { - fd = i2c_open_path(device_path, REGISTER_ADDRESS, 0); - free(device_path); - } else { - fd = i2c_open(bus_number, REGISTER_ADDRESS, 0); - } - + int fd = i2c_open_from_programmer_params(REGISTER_ADDRESS, 0); if (fd < 0) return fd; -- To view, visit https://review.coreboot.org/c/flashrom/+/52830 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I006b311c88feea37fe4b217f769b21ca1505def9 Gerrit-Change-Number: 52830 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: i2c_helper_linux: Clean up whitespace a bit
by Angel Pons (Code Review) 02 May '21

02 May '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52828 ) Change subject: i2c_helper_linux: Clean up whitespace a bit ...................................................................... i2c_helper_linux: Clean up whitespace a bit Use tabs to indent code and drop an unnecessary newline. Change-Id: I8fe4a8b9213677e0d0bee9681abf94726c934cc6 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M i2c_helper_linux.c 1 file changed, 1 insertion(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/28/52828/1 diff --git a/i2c_helper_linux.c b/i2c_helper_linux.c index 7744505..963c399 100644 --- a/i2c_helper_linux.c +++ b/i2c_helper_linux.c @@ -50,13 +50,12 @@ if (ret < 0) { msg_perr("Unable to set I2C slave address to 0x%02x: %s.\n", addr, strerror(errno)); i2c_close(fd); - return ret; + return ret; } return fd; } - int i2c_open(int bus, uint16_t addr, int force) { char dev[sizeof(I2C_DEV_PREFIX)] = {0}; -- To view, visit https://review.coreboot.org/c/flashrom/+/52828 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8fe4a8b9213677e0d0bee9681abf94726c934cc6 Gerrit-Change-Number: 52828 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: realtek_mst_i2c_spi: Add missing braces
by Angel Pons (Code Review) 02 May '21

02 May '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52827 ) Change subject: realtek_mst_i2c_spi: Add missing braces ...................................................................... realtek_mst_i2c_spi: Add missing braces As per the coding style, if one branch of a conditional statement needs braces, all other branches need to have braces as well. Change-Id: I69b762391165177857e9331f79f54b01149cf339 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M realtek_mst_i2c_spi.c 1 file changed, 10 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/27/52827/1 diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index ae79fdd..27bcb4a 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -476,30 +476,32 @@ reset_str = extract_programmer_param("reset-mcu"); if (reset_str) { - if (reset_str[0] == '1') + if (reset_str[0] == '1') { *reset = 1; - else if (reset_str[0] == '0') + } else if (reset_str[0] == '0') { *reset = 0; - else { + } else { msg_perr("%s: Incorrect param format, reset-mcu=1 or 0.\n", __func__); ret = SPI_GENERIC_ERROR; } - } else + } else { *reset = 0; /* Default behaviour is no MCU reset on tear-down. */ + } free(reset_str); isp_str = extract_programmer_param("enter-isp"); if (isp_str) { - if (isp_str[0] == '1') + if (isp_str[0] == '1') { *enter_isp = 1; - else if (isp_str[0] == '0') + } else if (isp_str[0] == '0') { *enter_isp = 0; - else { + } else { msg_perr("%s: Incorrect param format, enter-isp=1 or 0.\n", __func__); ret = SPI_GENERIC_ERROR; } - } else + } else { *enter_isp = 1; /* Default behaviour is enter ISP on setup. */ + } free(isp_str); _get_params_failed: -- To view, visit https://review.coreboot.org/c/flashrom/+/52827 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I69b762391165177857e9331f79f54b01149cf339 Gerrit-Change-Number: 52827 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: lspcon_i2c_spi: support a devpath option
by Angel Pons (Code Review) 02 May '21

02 May '21

Angel Pons has submitted this change. ( https://review.coreboot.org/c/flashrom/+/51967 ) Change subject: lspcon_i2c_spi: support a devpath option ...................................................................... lspcon_i2c_spi: support a devpath option Some callers may find it easier to provide the path to an I2C device at which to communicate with the device, rather than specify the bus number- doing so might involve trying to parse a path to extract the number only for flashrom to do the reverse, which is error-prone and unnecessary. This change adds support for a `devpath` option, continuing to allow `bus` and requiring only one of them be specified. TEST=Verified --flash-size outputs correct values with both devpath=/dev/i2c-7 and bus=7, as well as noting that one is required if neither is specified and only one may be specified if both are given. Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> Change-Id: Id2adf8a307b9205ce5e5804a6c3e22f19d0c34c9 Reviewed-on: https://review.coreboot.org/c/flashrom/+/51967 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M i2c_helper.h M i2c_helper_linux.c M lspcon_i2c_spi.c 3 files changed, 78 insertions(+), 50 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved diff --git a/i2c_helper.h b/i2c_helper.h index 599e08b..cab2ce6 100644 --- a/i2c_helper.h +++ b/i2c_helper.h @@ -72,6 +72,14 @@ int i2c_open(int bus, uint16_t addr, int force); /** + * i2c_open_path: open an I2C device by device path + * + * This function behaves the same as i2c_open, but takes a filesystem + * path (assumed to be an I2C device file) instead of a bus number. + */ +int i2c_open_path(const char *path, uint16_t addr, int force); + +/** * i2c_close - closes the file descriptor returned by i2c_open * * @fd: file descriptor to be closed. diff --git a/i2c_helper_linux.c b/i2c_helper_linux.c index 9383c85..7744505 100644 --- a/i2c_helper_linux.c +++ b/i2c_helper_linux.c @@ -37,14 +37,31 @@ return fd == -1 ? 0 : close(fd); } +int i2c_open_path(const char *path, uint16_t addr, int force) +{ + int fd = open(path, O_RDWR); + if (fd < 0) { + msg_perr("Unable to open I2C device %s: %s.\n", path, strerror(errno)); + return fd; + } + + int request = force ? I2C_SLAVE_FORCE : I2C_SLAVE; + int ret = ioctl(fd, request, addr); + if (ret < 0) { + msg_perr("Unable to set I2C slave address to 0x%02x: %s.\n", addr, strerror(errno)); + i2c_close(fd); + return ret; + } + + return fd; +} + + int i2c_open(int bus, uint16_t addr, int force) { char dev[sizeof(I2C_DEV_PREFIX)] = {0}; int ret = -1; - int fd = -1; - - int request = force ? I2C_SLAVE_FORCE : I2C_SLAVE; if (bus < 0 || bus > I2C_MAX_BUS) { msg_perr("Invalid I2C bus %d.\n", bus); @@ -57,20 +74,7 @@ return ret; } - fd = open(dev, O_RDWR); - if (fd < 0) { - msg_perr("Unable to open I2C device %s: %s.\n", dev, strerror(errno)); - return fd; - } - - ret = ioctl(fd, request, addr); - if (ret < 0) { - msg_perr("Unable to set I2C slave address to 0x%02x: %s.\n", addr, strerror(errno)); - i2c_close(fd); - return ret; - } - - return fd; + return i2c_open_path(dev, addr, force); } int i2c_read(int fd, uint16_t addr, i2c_buffer_t *buf) diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c index 7599db6..b79e7f7 100644 --- a/lspcon_i2c_spi.c +++ b/lspcon_i2c_spi.c @@ -434,56 +434,72 @@ } /* TODO: remove this out of the specific SPI master implementation. */ -static int get_bus(void) +static int get_bus_number(void) { char *bus_str = extract_programmer_param("bus"); - int ret = SPI_GENERIC_ERROR; - if (bus_str) { - char *bus_suffix; - errno = 0; - int bus = (int)strtol(bus_str, &bus_suffix, 10); - if (errno != 0 || bus_str == bus_suffix) { - msg_perr("%s: Could not convert 'bus'.\n", __func__); - goto get_bus_done; - } + /* Return INVALID_ADDRESS if bus value was given but invalid, and GENERIC_ERROR + * if no value was provided. */ + int ret = SPI_INVALID_ADDRESS; - if (bus < 0 || bus > 255) { - msg_perr("%s: Value for 'bus' is out of range(0-255).\n", - __func__); - goto get_bus_done; - } + if (bus_str == NULL) + return SPI_GENERIC_ERROR; - if (strlen(bus_suffix) > 0) { - msg_perr("%s: Garbage following 'bus' value.\n", - __func__); - goto get_bus_done; - } - - msg_pinfo("Using i2c bus %i.\n", bus); - ret = bus; + char *bus_suffix; + errno = 0; + int bus = (int)strtol(bus_str, &bus_suffix, 10); + if (errno != 0 || bus_str == bus_suffix) { + msg_perr("%s: Could not convert 'bus'.\n", __func__); goto get_bus_done; - } else { - msg_perr("%s: Bus number not specified.\n", __func__); } -get_bus_done: - if (bus_str) - free(bus_str); + if (bus < 0 || bus > 255) { + msg_perr("%s: Value for 'bus' is out of range(0-255).\n", __func__); + goto get_bus_done; + } + + if (strlen(bus_suffix) > 0) { + msg_perr("%s: Garbage following 'bus' value.\n", __func__); + goto get_bus_done; + } + + msg_pinfo("Using i2c bus %i.\n", bus); + ret = bus; + +get_bus_done: + free(bus_str); return ret; } int lspcon_i2c_spi_init(void) { - int lspcon_i2c_spi_bus = get_bus(); - if (lspcon_i2c_spi_bus < 0) + int fd = -1; + int bus_number = get_bus_number(); + if (bus_number == SPI_INVALID_ADDRESS) { + /* Bus was specified but unusable, bail out immediately */ return SPI_GENERIC_ERROR; + } + char *device_path = extract_programmer_param("devpath"); - int ret = 0; - int fd = i2c_open(lspcon_i2c_spi_bus, REGISTER_ADDRESS, 0); + if (device_path != NULL && bus_number >= 0) { + msg_perr("%s: only one of bus and devpath may be specified\n", __func__); + free(device_path); + return SPI_GENERIC_ERROR; + } else if (device_path == NULL && bus_number < 0) { + msg_perr("%s: one of bus and devpath must be specified\n", __func__); + return SPI_GENERIC_ERROR; + } + + if (device_path != NULL) { + fd = i2c_open_path(device_path, REGISTER_ADDRESS, 0); + free(device_path); + } else { + fd = i2c_open(bus_number, REGISTER_ADDRESS, 0); + } + if (fd < 0) return fd; - ret |= lspcon_i2c_spi_reset_mpu_stop(fd); + int ret = lspcon_i2c_spi_reset_mpu_stop(fd); if (ret) { msg_perr("%s: call to reset_mpu_stop failed.\n", __func__); return ret; -- To view, visit https://review.coreboot.org/c/flashrom/+/51967 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id2adf8a307b9205ce5e5804a6c3e22f19d0c34c9 Gerrit-Change-Number: 51967 Gerrit-PatchSet: 7 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Campello <campello(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021