- flashrom-gerrit - mail.coreboot.org

[L] Change in flashrom[master]: ch347_spi.c: Add initial support for the WCH CH347
by Nicholas Chin (Code Review) 11 Dec '22

11 Dec '22

Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk. Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70573 ) Change subject: ch347_spi.c: Add initial support for the WCH CH347 ...................................................................... Patch Set 3: (6 comments) File ch347_spi.c: https://review.coreboot.org/c/flashrom/+/70573/comment/7bde1c88_74ca3b6b PS2, Line 24: #define CH347_CMD_SPI_CFG 0xC0 : #define CH347_CMD_SPI_CS_CTRL 0xC1 : #define CH347_CMD_SPI_OUT_IN 0xC2 : #define CH347_CMD_SPI_IN 0xC3 : #define CH347_CMD_SPI_OUT 0xC4 > Suggestion: use tabs to align the hex values? […] Done. I used mixed spaces and tabs to get it to align properly even when using different tab widths in a text editor, not sure if that's the best way to do it. https://review.coreboot.org/c/flashrom/+/70573/comment/13686991_a4e02a57 PS2, Line 48: static struct libusb_device_handle *handle = NULL; > There have been changes to other programmers to pass this kind of data around through the flashctx. […] Done https://review.coreboot.org/c/flashrom/+/70573/comment/b0a2bac7_b9e3ef53 PS2, Line 51: 1 > Do you need to specify the array size? Or is this hardcoded because the code below only tries openin […] Done. Removed the array size and added a terminating {0} entry which other programmers include. https://review.coreboot.org/c/flashrom/+/70573/comment/4ee00a36_504b7a33 PS2, Line 71: uint8_t cmd[13] = {0}; : cmd[0] = CH347_CMD_SPI_CS_CTRL; : /* payload length, uint16 LSB: 10 */ : cmd[1] = 10; : : cmd[3] = cs1; : cmd[8] = cs2; > How about: […] Done. I forgot about this syntax, thanks! https://review.coreboot.org/c/flashrom/+/70573/comment/8516c7c7_03fec2d2 PS2, Line 221: > nit: trailing space Done https://review.coreboot.org/c/flashrom/+/70573/comment/ce33b794_89ee63d2 PS2, Line 287: uint16_t vid = devs_ch347_spi[0].vendor_id; : uint16_t pid = devs_ch347_spi[0].device_id; : handle = libusb_open_device_with_vid_pid(NULL, vid, pid); : if (handle == NULL) { : msg_perr("Couldn't open device %04x:%04x.\n", vid, pid); : return -1; : } > Does the CH341A code do the same? It should be fine as long as there's only one VID/DID in the list. Yeah, it does. I took a look at other libusb programmers and most of them do basically the same thing as most only match a single VID/DID. It looks like stlinkv3_spi is the only libusb programmer which matches multiple IDs. I'll take a look at how that works when I add support for the CH347 HID mode which uses a different DID. -- To view, visit https://review.coreboot.org/c/flashrom/+/70573 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I31b86c41076cc45d4a416a73fa1131350fb745ba Gerrit-Change-Number: 70573 Gerrit-PatchSet: 3 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 11 Dec 2022 19:52:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: ch347_spi.c: Add initial support for the WCH CH347
by Nicholas Chin (Code Review) 11 Dec '22

11 Dec '22

Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk, Nicholas Chin. Hello build bot (Jenkins), Thomas Heijligen, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70573 to look at the new patch set (#3). Change subject: ch347_spi.c: Add initial support for the WCH CH347 ...................................................................... ch347_spi.c: Add initial support for the WCH CH347 Add support for the WCH CH347, a high-speed USB to bus converter supporting multiple protocols interfaces including SPI. Currently only mode 1 (vendor defined communication interface) is supported, mode 2 (USB HID communication interface) support will be added later. The code is currently hard coded to use a CS1 and a SPI clock of 60 MHz, though there are 2 CS lines and 6 other GPIO lines available, as well as a configurable clock divisor. Support for these will be exposed through programmer parameters in later commits. This currently uses the synchronous libusb API. Performance seems to be alright so far, if it becomes an issue I may switch to the asynchronous API. Tested with a MX25L1606E flash chip and a hard coded divisor of 2 for a SPI clock speed of 15 MHz, as I was having signal integrity issues at higher clock speeds. Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com> Change-Id: I31b86c41076cc45d4a416a73fa1131350fb745ba --- M Makefile A ch347_spi.c M include/programmer.h M programmer_table.c 4 files changed, 393 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/73/70573/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/70573 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I31b86c41076cc45d4a416a73fa1131350fb745ba Gerrit-Change-Number: 70573 Gerrit-PatchSet: 3 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: serial: Add Darwin/macOS support for custom and >230400 baudrates
by Peter Stuge (Code Review) 11 Dec '22

11 Dec '22

Attention is currently required from: Thomas Heijligen, Name of user not set #1004601. Hello build bot (Jenkins), Thomas Heijligen, Name of user not set #1004601, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70571 to look at the new patch set (#4). Change subject: serial: Add Darwin/macOS support for custom and >230400 baudrates ...................................................................... serial: Add Darwin/macOS support for custom and >230400 baudrates This change is based on the patch proposed by Denis Ahrens in https://review.coreboot.org/c/flashrom/+/67822 Change-Id: I3e6b88d2b4c2a130b16456752681fd9f807bf6f0 Signed-off-by: Peter Stuge <peter(a)stuge.se> --- M Makefile A custom_baud_darwin.c M meson.build M serial.c 4 files changed, 80 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/71/70571/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/70571 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3e6b88d2b4c2a130b16456752681fd9f807bf6f0 Gerrit-Change-Number: 70571 Gerrit-PatchSet: 4 Gerrit-Owner: Peter Stuge <peter(a)stuge.se> Gerrit-Reviewer: Name of user not set #1004601 Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Name of user not set #1004601 Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: serial: Add Darwin/macOS support for custom and >230400 baudrates
by Peter Stuge (Code Review) 11 Dec '22

11 Dec '22

Attention is currently required from: Thomas Heijligen, Name of user not set #1004601. Hello build bot (Jenkins), Thomas Heijligen, Name of user not set #1004601, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70571 to look at the new patch set (#3). Change subject: serial: Add Darwin/macOS support for custom and >230400 baudrates ...................................................................... serial: Add Darwin/macOS support for custom and >230400 baudrates This change is based on the patch proposed by Denis Ahrens in https://review.coreboot.org/c/flashrom/+/67822 Change-Id: I3e6b88d2b4c2a130b16456752681fd9f807bf6f0 Signed-off-by: Peter Stuge <peter(a)stuge.se> --- M Makefile A custom_baud_darwin.c M meson.build M serial.c 4 files changed, 87 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/71/70571/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/70571 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3e6b88d2b4c2a130b16456752681fd9f807bf6f0 Gerrit-Change-Number: 70571 Gerrit-PatchSet: 3 Gerrit-Owner: Peter Stuge <peter(a)stuge.se> Gerrit-Reviewer: Name of user not set #1004601 Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Name of user not set #1004601 Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: libflashrom.c: Invert if conditions to improve the readability
by Felix Singer (Code Review) 11 Dec '22

11 Dec '22

Felix Singer has submitted this change. ( https://review.coreboot.org/c/flashrom/+/68153 ) Change subject: libflashrom.c: Invert if conditions to improve the readability ...................................................................... libflashrom.c: Invert if conditions to improve the readability Invert some if conditions to improve the readability of the code. Instead of running some code if the specific condition applies, error out early and reduce the indentation levels. Also, while at it, move the initializers for these for-loops iterators into their constructs. They are only used by them. Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b Signed-off-by: Felix Singer <felixsinger(a)posteo.net> Reviewed-on: https://review.coreboot.org/c/flashrom/+/68153 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M libflashrom.c 1 file changed, 57 insertions(+), 36 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/libflashrom.c b/libflashrom.c index c83c1bd..44f297f 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -91,26 +91,26 @@ struct flashrom_flashchip_info *flashrom_supported_flash_chips(void) { - unsigned int i = 0; struct flashrom_flashchip_info *supported_flashchips = malloc(flashchips_size * sizeof(*supported_flashchips)); - if (supported_flashchips != NULL) { - for (; i < flashchips_size; ++i) { - supported_flashchips[i].vendor = flashchips[i].vendor; - supported_flashchips[i].name = flashchips[i].name; - supported_flashchips[i].tested.erase = - (enum flashrom_test_state)flashchips[i].tested.erase; - supported_flashchips[i].tested.probe = - (enum flashrom_test_state)flashchips[i].tested.probe; - supported_flashchips[i].tested.read = - (enum flashrom_test_state)flashchips[i].tested.read; - supported_flashchips[i].tested.write = - (enum flashrom_test_state)flashchips[i].tested.write; - supported_flashchips[i].total_size = flashchips[i].total_size; - } - } else { + if (!supported_flashchips) { msg_gerr("Memory allocation error!\n"); + return NULL; + } + + for (unsigned int i = 0; i < flashchips_size; ++i) { + supported_flashchips[i].vendor = flashchips[i].vendor; + supported_flashchips[i].name = flashchips[i].name; + supported_flashchips[i].tested.erase = + (enum flashrom_test_state)flashchips[i].tested.erase; + supported_flashchips[i].tested.probe = + (enum flashrom_test_state)flashchips[i].tested.probe; + supported_flashchips[i].tested.read = + (enum flashrom_test_state)flashchips[i].tested.read; + supported_flashchips[i].tested.write = + (enum flashrom_test_state)flashchips[i].tested.write; + supported_flashchips[i].total_size = flashchips[i].total_size; } return supported_flashchips; @@ -120,7 +120,6 @@ { #if CONFIG_INTERNAL == 1 int boards_known_size = 0; - int i = 0; const struct board_info *binfo = boards_known; while ((binfo++)->vendor) @@ -132,15 +131,16 @@ struct flashrom_board_info *supported_boards = malloc(boards_known_size * sizeof(*supported_boards)); - if (supported_boards != NULL) { - for (; i < boards_known_size; ++i) { - supported_boards[i].vendor = binfo[i].vendor; - supported_boards[i].name = binfo[i].name; - supported_boards[i].working = - (enum flashrom_test_state) binfo[i].working; - } - } else { + if (!supported_boards) { msg_gerr("Memory allocation error!\n"); + return NULL; + } + + for (int i = 0; i < boards_known_size; ++i) { + supported_boards[i].vendor = binfo[i].vendor; + supported_boards[i].name = binfo[i].name; + supported_boards[i].working = + (enum flashrom_test_state) binfo[i].working; } return supported_boards; @@ -153,7 +153,6 @@ { #if CONFIG_INTERNAL == 1 int chipset_enables_size = 0; - int i = 0; const struct penable *chipset = chipset_enables; while ((chipset++)->vendor_name) @@ -165,17 +164,18 @@ struct flashrom_chipset_info *supported_chipsets = malloc(chipset_enables_size * sizeof(*supported_chipsets)); - if (supported_chipsets != NULL) { - for (; i < chipset_enables_size; ++i) { - supported_chipsets[i].vendor = chipset[i].vendor_name; - supported_chipsets[i].chipset = chipset[i].device_name; - supported_chipsets[i].vendor_id = chipset[i].vendor_id; - supported_chipsets[i].chipset_id = chipset[i].device_id; - supported_chipsets[i].status = - (enum flashrom_test_state) chipset[i].status; - } - } else { + if (!supported_chipsets) { msg_gerr("Memory allocation error!\n"); + return NULL; + } + + for (int i = 0; i < chipset_enables_size; ++i) { + supported_chipsets[i].vendor = chipset[i].vendor_name; + supported_chipsets[i].chipset = chipset[i].device_name; + supported_chipsets[i].vendor_id = chipset[i].vendor_id; + supported_chipsets[i].chipset_id = chipset[i].device_id; + supported_chipsets[i].status = + (enum flashrom_test_state) chipset[i].status; } return supported_chipsets; -- To view, visit https://review.coreboot.org/c/flashrom/+/68153 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b Gerrit-Change-Number: 68153 Gerrit-PatchSet: 7 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: tests/selfcheck.c: Fix on non-x86 machines
by Angel Pons (Code Review) 11 Dec '22

11 Dec '22

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk, Evan Benn. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70554 ) Change subject: tests/selfcheck.c: Fix on non-x86 machines ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: ``` In file included from ../../tests/include/test.h:28, from ../../tests/selfcheck.c:19: ../../tests/selfcheck.c: In function 'selfcheck_board_matches_table': ../../tests/selfcheck.c:142:21: error: 'board_matches_size' undeclared (first use in this function) 142 | assert_true(board_matches_size > 1); | ^~~~~~~~~~~~~~~~~~ ../../tests/selfcheck.c:142:21: note: each undeclared identifier is reported only once for each function it appears in ../../tests/selfcheck.c:143:21: error: 'board_matches' undeclared (first use in this function) 143 | assert_true(board_matches[board_matches_size - 1].vendor_name == NULL); ``` Looks like some more things need to be guarded. -- To view, visit https://review.coreboot.org/c/flashrom/+/70554 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Icbe677d3ef164e998daf898ddbea34f96246677f Gerrit-Change-Number: 70554 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Sun, 11 Dec 2022 11:05:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: libflashrom.c: Invert if conditions to improve the readability
by Angel Pons (Code Review) 11 Dec '22

11 Dec '22

Attention is currently required from: Felix Singer. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68153 ) Change subject: libflashrom.c: Invert if conditions to improve the readability ...................................................................... Patch Set 6: (1 comment) File libflashrom.c: https://review.coreboot.org/c/flashrom/+/68153/comment/db280cff_c4fe1e5a PS3, Line 107: (enum flashrom_test_state) > So yes, the cast is needed for now. I'm setting this to resolved since it's unrelated. No idea why we have 2 enums for the same thing. We shouldn't. -- To view, visit https://review.coreboot.org/c/flashrom/+/68153 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b Gerrit-Change-Number: 68153 Gerrit-PatchSet: 6 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> 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-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Comment-Date: Sun, 11 Dec 2022 11:04:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: libflashrom.c: Invert if conditions to improve the readability
by Angel Pons (Code Review) 11 Dec '22

11 Dec '22

Attention is currently required from: Felix Singer. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68153 ) Change subject: libflashrom.c: Invert if conditions to improve the readability ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68153 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b Gerrit-Change-Number: 68153 Gerrit-PatchSet: 6 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> 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-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Comment-Date: Sun, 11 Dec 2022 11:02:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: ch347_spi.c: Initial support for the WCH CH347
by Angel Pons (Code Review) 11 Dec '22

11 Dec '22

Attention is currently required from: Thomas Heijligen, Nicholas Chin. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70573 ) Change subject: ch347_spi.c: Initial support for the WCH CH347 ...................................................................... Patch Set 2: Code-Review+1 (6 comments) File ch347_spi.c: https://review.coreboot.org/c/flashrom/+/70573/comment/d4d56a34_ec6727d6 PS2, Line 24: #define CH347_CMD_SPI_CFG 0xC0 : #define CH347_CMD_SPI_CS_CTRL 0xC1 : #define CH347_CMD_SPI_OUT_IN 0xC2 : #define CH347_CMD_SPI_IN 0xC3 : #define CH347_CMD_SPI_OUT 0xC4 Suggestion: use tabs to align the hex values? ``` #define CH347_CMD_SPI_CFG 0xC0 #define CH347_CMD_SPI_CS_CTRL 0xC1 #define CH347_CMD_SPI_OUT_IN 0xC2 #define CH347_CMD_SPI_IN 0xC3 #define CH347_CMD_SPI_OUT 0xC4 ``` https://review.coreboot.org/c/flashrom/+/70573/comment/63330009_bb1e3c85 PS2, Line 48: static struct libusb_device_handle *handle = NULL; There have been changes to other programmers to pass this kind of data around through the flashctx. It'd be nice to do so here as well. https://review.coreboot.org/c/flashrom/+/70573/comment/70c9c377_3f928e2e PS2, Line 51: 1 Do you need to specify the array size? Or is this hardcoded because the code below only tries opening a device with this ID? https://review.coreboot.org/c/flashrom/+/70573/comment/668a20d3_272d8b52 PS2, Line 71: uint8_t cmd[13] = {0}; : cmd[0] = CH347_CMD_SPI_CS_CTRL; : /* payload length, uint16 LSB: 10 */ : cmd[1] = 10; : : cmd[3] = cs1; : cmd[8] = cs2; How about: ``` uint8_t cmd[13] = { [0] = CH347_CMD_SPI_CS_CTRL, /* payload length, uint16 LSB: 10 */ [1] = 10, [3] = cs1, [8] = cs2, }; ``` https://review.coreboot.org/c/flashrom/+/70573/comment/d3e14433_b2a08e5d PS2, Line 221: nit: trailing space https://review.coreboot.org/c/flashrom/+/70573/comment/455c3f1a_f0b22586 PS2, Line 287: uint16_t vid = devs_ch347_spi[0].vendor_id; : uint16_t pid = devs_ch347_spi[0].device_id; : handle = libusb_open_device_with_vid_pid(NULL, vid, pid); : if (handle == NULL) { : msg_perr("Couldn't open device %04x:%04x.\n", vid, pid); : return -1; : } Does the CH341A code do the same? It should be fine as long as there's only one VID/DID in the list. -- To view, visit https://review.coreboot.org/c/flashrom/+/70573 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I31b86c41076cc45d4a416a73fa1131350fb745ba Gerrit-Change-Number: 70573 Gerrit-PatchSet: 2 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Comment-Date: Sun, 11 Dec 2022 11:01:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: ch347_spi.c: Initial support for the WCH CH347
by Nicholas Chin (Code Review) 11 Dec '22

11 Dec '22

Attention is currently required from: Thomas Heijligen. Hello build bot (Jenkins), Thomas Heijligen, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70573 to look at the new patch set (#2). Change subject: ch347_spi.c: Initial support for the WCH CH347 ...................................................................... ch347_spi.c: Initial support for the WCH CH347 Add support for the WCH CH347, a high-speed USB to bus converter supporting multiple protocols interfaces including SPI. Currently only mode 1 (vendor defined communication interface) is supported, mode 2 (USB HID communication interface) support will be added later. The code is currently hard coded to use a CS1 and a SPI clock of 60 MHz, though there are 2 CS lines and 6 other GPIO lines available, as well as a configurable clock divisor. Support for these will be exposed through programmer parameters in later commits. This currently uses the synchronous libusb API. Performance seems to be alright so far, if it becomes an issue I may switch to the asynchronous API. Tested with a MX25L1606E flash chip and a hard coded divisor of 2 for a SPI clock speed of 15 MHz, as I was having signal integrity issues at higher clock speeds. Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com> Change-Id: I31b86c41076cc45d4a416a73fa1131350fb745ba --- M Makefile A ch347_spi.c M include/programmer.h M programmer_table.c 4 files changed, 388 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/73/70573/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70573 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I31b86c41076cc45d4a416a73fa1131350fb745ba Gerrit-Change-Number: 70573 Gerrit-PatchSet: 2 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newpatchset

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit