December 2022 - flashrom-gerrit

[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

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

11 Dec '22

Attention is currently required from: Felix Singer. Edward O'Callaghan 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 07:21:01 +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 Nicholas Chin (Code Review) 11 Dec '22

11 Dec '22

Nicholas Chin has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70573 ) 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 3 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/1 diff --git a/Makefile b/Makefile index 425b58c..955ce4b 100644 --- a/Makefile +++ b/Makefile @@ -155,6 +155,7 @@ DEPENDS_ON_LIBUSB1 := \ CONFIG_CH341A_SPI \ + CONFIG_CH347_SPI \ CONFIG_DEDIPROG \ CONFIG_DEVELOPERBOX_SPI \ CONFIG_DIGILENT_SPI \ @@ -516,6 +517,9 @@ # Winchiphead CH341A CONFIG_CH341A_SPI ?= yes +# Winchiphead CH347 +CONFIG_CH347_SPI ?= yes + # Digilent Development board JTAG CONFIG_DIGILENT_SPI ?= yes @@ -767,6 +771,11 @@ PROGRAMMER_OBJS += ch341a_spi.o endif +ifeq ($(CONFIG_CH347_SPI), yes) +FEATURE_FLAGS += -D'CONFIG_CH347_SPI=1' +PROGRAMMER_OBJS += ch347_spi.o +endif + ifeq ($(CONFIG_DIGILENT_SPI), yes) FEATURE_FLAGS += -D'CONFIG_DIGILENT_SPI=1' PROGRAMMER_OBJS += digilent_spi.o diff --git a/ch347_spi.c b/ch347_spi.c new file mode 100644 index 0000000..f8c7edd --- /dev/null +++ b/ch347_spi.c @@ -0,0 +1,347 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2022 Nicholas Chin <nic.c3.14(a)gmail.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <string.h> +#include <stdlib.h> +#include <libusb.h> +#include "platform.h" +#include "programmer.h" +#include "flash.h" + +#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 + +#define CH347_CS_ASSERT 0x00 +#define CH347_CS_DEASSERT 0x40 +#define CH347_CS_CHANGE 0x80 +#define CH347_CS_IGNORE 0x00 + +#define WRITE_EP 0x06 +#define READ_EP 0x86 + +#define MODE_1_IFACE 2 +#define MODE_2_IFACE 1 + +/* The USB descriptor says the max transfer size is 512 bytes, but the + * vendor driver only seems to transfer a maximum of 510 bytes at once, + * leaving 507 bytes for data as the command + length take up 3 bytes + */ +#define CH347_MAX_DATA_LEN 507 + + +static struct libusb_device_handle *handle = NULL; + +/* TODO: Add support for HID mode */ +static const struct dev_entry devs_ch347_spi[1] = { + {0x1A86, 0x55DB, OK, "QinHeng Electronics", "USB To UART+SPI+I2C"} +}; + +static int ch347_spi_shutdown(void *data) +{ + if (handle == NULL) + return -1; + /* TODO: Set this depending on the mode */ + int spi_interface = MODE_1_IFACE; + libusb_release_interface(handle, spi_interface); + libusb_attach_kernel_driver(handle, spi_interface); + libusb_close(handle); + libusb_exit(NULL); + handle = NULL; + return 0; +} + +static int ch347_set_cs(uint8_t cs1, uint8_t cs2) +{ + 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; + + int32_t ret = libusb_bulk_transfer(handle, WRITE_EP, cmd, sizeof(cmd), NULL, 1000); + if (ret < 0) { + msg_perr("Could not change CS!\n"); + return -1; + } + return 0; +} + + +static int ch347_write(unsigned int writecnt, const uint8_t *writearr) +{ + unsigned int data_len = writecnt; + uint8_t resp_buf[4] = {0}; + int packet_len = 0; + unsigned int i = 0; + int ret = 0; + int transferred = 0; + if (data_len > CH347_MAX_DATA_LEN) { + data_len = CH347_MAX_DATA_LEN; + } + packet_len = data_len + 3; + + /* 3 bytes for command + data length */ + uint8_t *buffer = malloc(packet_len); + if (buffer == NULL) { + msg_perr("Could not allocate write buffer\n"); + } + + while (i < writecnt) { + buffer[0] = CH347_CMD_SPI_OUT; + buffer[1] = (data_len) & 0xFF; + buffer[2] = ((data_len) & 0xFF00) >> 8; + memcpy(buffer + 3, writearr + i, data_len); + ret = libusb_bulk_transfer(handle, WRITE_EP, buffer, packet_len, &transferred, 1000); + if (ret < 0 || transferred != packet_len) { + msg_perr("Could not write\n"); + return -1; + } + ret = libusb_bulk_transfer(handle, READ_EP, resp_buf, sizeof(resp_buf), NULL, 1000); + if (ret < 0) { + msg_perr("Could not read write response\n"); + return -1; + } + i += data_len; + data_len = writecnt - i; + if (data_len > CH347_MAX_DATA_LEN) { + data_len = CH347_MAX_DATA_LEN; + } + packet_len = data_len + 3; + } + return 0; +} + +static int ch347_read(unsigned int readcnt, uint8_t *readarr) +{ + uint8_t *read_ptr = readarr; + uint8_t command_buf[7] = {0}; + int ret = 0; + int transferred = 0; + unsigned int read = 0; + + /* 3 bytes for command + data length */ + uint8_t *buffer = malloc(CH347_MAX_DATA_LEN + 3); + if (buffer == NULL) { + msg_perr("Could not allocate read buffer\n"); + } + command_buf[0] = CH347_CMD_SPI_IN; + command_buf[1] = 4; + command_buf[2] = 0; + command_buf[3] = readcnt & 0xFF; + command_buf[4] = (readcnt & 0xFF00) >> 8; + command_buf[5] = (readcnt & 0xFF0000) >> 16; + command_buf[6] = (readcnt & 0xFF000000) >> 24; + + ret = libusb_bulk_transfer(handle, WRITE_EP, command_buf, sizeof(command_buf), &transferred, 1000); + if (ret < 0 || transferred != sizeof(command_buf)) { + msg_perr("Could not send read command\n"); + return -1; + } + + while (read < readcnt) { + ret = libusb_bulk_transfer(handle, READ_EP, buffer, CH347_MAX_DATA_LEN + 3, NULL, 1000); + if (ret < 0) { + printf(libusb_error_name(ret)); + msg_perr("Could not read data\n"); + return -1; + } + transferred = read_le16(buffer, 1); + read += transferred; + if (read > readcnt) { + msg_perr("CH347 returned more bytes than requested!"); + return -1; + } + memcpy(read_ptr, buffer + 3, transferred); + read_ptr += transferred; + } + return 0; +} + +static int ch347_spi_send_command(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) +{ + int ret = 0; + + ch347_set_cs(CH347_CS_ASSERT | CH347_CS_CHANGE, CH347_CS_IGNORE); + if (writecnt) { + ret = ch347_write(writecnt, writearr); + if (ret < 0) { + perror("CH347 write error\n"); + return -1; + } + } + if (readcnt) { + ret = ch347_read(readcnt, readarr); + if (ret < 0) { + perror("CH347 read error\n"); + return -1; + } + } + ch347_set_cs(CH347_CS_DEASSERT | CH347_CS_CHANGE, CH347_CS_IGNORE); + + return 0; +} + +static int32_t ch347_spi_config(uint8_t divisor) +{ + if (handle == NULL) + return -1; + uint8_t read_cfg_cmd[] = {0xCA, 0x01, 0x00, 0x01}; + uint8_t buff[29] = {0}; + int32_t ret = libusb_bulk_transfer(handle, WRITE_EP, read_cfg_cmd, sizeof(read_cfg_cmd), NULL, 1000); + if (ret < 0) { + msg_perr("Could not send read config command!\n"); + } + ret = libusb_bulk_transfer(handle, READ_EP, buff, sizeof(buff), NULL, 1000); + if (ret < 0) { + msg_perr("Could not read CH347 config!\n"); + } + buff[0] = CH347_CMD_SPI_CFG; + buff[1] = (sizeof(buff)-3) & 0xFF; + buff[2] = ((sizeof(buff)-3) & 0xFF00) >> 8; + /* Not sure what these two bytes do, but the vendor + * drivers seem to unconditionally set these values + */ + buff[5] = 4; + buff[6] = 1; + /* Clock polarity: bit 1 */ + buff[9] = 0; + /* Clock phase: bit 0 */ + buff[11] = 0; + /* Another mystery byte */ + buff[14] = 2; + /* Clock divisor: bits 5:3 */ + buff[15] = (divisor & 0x7) << 3; + /* Bit order: bit 7, 0=MSB */ + buff[17] = 0; + /* Yet another mystery byte */ + buff[19] = 7; + /* CS polarity: bit 7 CS2, bit 6 CS1. 0 = active low */ + buff[24] = 0; + + ret = libusb_bulk_transfer(handle, WRITE_EP, buff, sizeof(buff), NULL, 1000); + if (ret < 0) { + msg_perr("Could not configure SPI interface!\n"); + } + /* FIXME: Not sure if the CH347 sends error responses for + * invalid config data, if so the code should check + */ + ret = libusb_bulk_transfer(handle, READ_EP, buff, sizeof(buff), NULL, 1000); + if (ret < 0) { + msg_perr("Could not read response!\n"); + } + return ret; +} + +static const struct spi_master spi_master_ch347_spi = { + .features = SPI_MASTER_4BA, + .max_data_read = 64 * 1024, + .max_data_write = 64 * 1024, + .command = ch347_spi_send_command, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, + .write_aai = default_spi_write_aai, + .shutdown = ch347_spi_shutdown, + .probe_opcode = default_spi_probe_opcode, +}; + +/* Largely copied from ch341a_spi.c */ +static int ch347_spi_init(const struct programmer_cfg *cfg) +{ + if (handle != NULL) { + msg_cerr("%s: handle already set! Please report a bug at flashrom(a)flashrom.org\n", __func__); + return -1; + } + + int32_t ret = libusb_init(NULL); + if (ret < 0) { + msg_perr("Couldn't initialize libusb!\n"); + return -1; + } + + /* Enable information, warning, and error messages (only). */ +#if LIBUSB_API_VERSION < 0x01000106 + libusb_set_debug(NULL, 3); +#else + libusb_set_option(NULL, LIBUSB_OPTION_LOG_LEVEL, LIBUSB_LOG_LEVEL_INFO); +#endif + + 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; + } + + /* TODO: set based on mode */ + /* Mode 1 uses interface 2 for the SPI interface */ + int spi_interface = MODE_1_IFACE; + + ret = libusb_detach_kernel_driver(handle, spi_interface); + if (ret != 0 && ret != LIBUSB_ERROR_NOT_FOUND) + msg_pwarn("Cannot detach the existing USB driver. Claiming the interface may fail. %s\n", + libusb_error_name(ret)); + ret = libusb_claim_interface(handle, spi_interface); + if (ret != 0) { + msg_perr("Failed to claim interface 2: '%s'\n", libusb_error_name(ret)); + goto close_handle; + } + + struct libusb_device *dev; + if (!(dev = libusb_get_device(handle))) { + msg_perr("Failed to get device from device handle.\n"); + goto close_handle; + } + + struct libusb_device_descriptor desc; + ret = libusb_get_device_descriptor(dev, &desc); + if (ret < 0) { + msg_perr("Failed to get device descriptor: '%s'\n", libusb_error_name(ret)); + goto release_interface; + } + + msg_pdbg("Device revision is %d.%01d.%01d\n", + (desc.bcdDevice >> 8) & 0x00FF, + (desc.bcdDevice >> 4) & 0x000F, + (desc.bcdDevice >> 0) & 0x000F); + + /* TODO: add programmer cfg for things like CS pin and divisor */ + if (ch347_spi_config(0) < 0) + goto release_interface; + + return register_spi_master(&spi_master_ch347_spi, NULL); + +release_interface: + libusb_release_interface(handle, spi_interface); +close_handle: + libusb_attach_kernel_driver(handle, spi_interface); + libusb_close(handle); + handle = NULL; + return -1; +} + +const struct programmer_entry programmer_ch347_spi = { + .name = "ch347_spi", + .type = USB, + .devs.dev = devs_ch347_spi, + .init = ch347_spi_init, +}; diff --git a/include/programmer.h b/include/programmer.h index 7bcef02..b8e87bc 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -62,6 +62,7 @@ extern const struct programmer_entry programmer_atavia; extern const struct programmer_entry programmer_buspirate_spi; extern const struct programmer_entry programmer_ch341a_spi; +extern const struct programmer_entry programmer_ch347_spi; extern const struct programmer_entry programmer_dediprog; extern const struct programmer_entry programmer_developerbox; extern const struct programmer_entry programmer_digilent_spi; diff --git a/programmer_table.c b/programmer_table.c index d58a155..954ed5a 100644 --- a/programmer_table.c +++ b/programmer_table.c @@ -152,6 +152,10 @@ &programmer_ch341a_spi, #endif +#if CONFIG_CH347_SPI == 1 + &programmer_ch347_spi, +#endif + #if CONFIG_DIGILENT_SPI == 1 &programmer_digilent_spi, #endif -- 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: 1 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-MessageType: newchange

1 0

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

11 Dec '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70554 ) Change subject: tests/selfcheck.c: Fix on non-x86 machines ...................................................................... tests/selfcheck.c: Fix on non-x86 machines The global const of `board_matches_size` has value `1` on non-x86 machines. Change-Id: Icbe677d3ef164e998daf898ddbea34f96246677f Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M tests/selfcheck.c 1 file changed, 14 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/70554/1 diff --git a/tests/selfcheck.c b/tests/selfcheck.c index aeccf18..40c9776 100644 --- a/tests/selfcheck.c +++ b/tests/selfcheck.c @@ -132,7 +132,7 @@ } } -#if CONFIG_INTERNAL == 1 +#if CONFIG_INTERNAL == 1 || defined(__i386__) || defined(__x86_64__) void selfcheck_board_matches_table(void **state) { (void)state; /* unused */ -- 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-MessageType: newchange

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. Peter Stuge has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70571 ) 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/1 diff --git a/Makefile b/Makefile index 729ff7a..3ae6c8a 100644 --- a/Makefile +++ b/Makefile @@ -803,9 +803,13 @@ ifeq ($(TARGET_OS), Linux) LIB_OBJS += custom_baud_linux.o else +ifeq ($(TARGET_OS), Darwin) +LIB_OBJS += custom_baud_darwin.o +else LIB_OBJS += custom_baud.o endif endif +endif USE_SOCKETS := $(if $(call filter_deps,$(DEPENDS_ON_SOCKETS)),yes,no) ifeq ($(USE_SOCKETS), yes) diff --git a/custom_baud_darwin.c b/custom_baud_darwin.c new file mode 100644 index 0000000..7525ac2 --- /dev/null +++ b/custom_baud_darwin.c @@ -0,0 +1,62 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2022 Peter Stuge <peter(a)stuge.se> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <termios.h> +#include <sys/ioctl.h> +#include <IOKit/serial/ioss.h> +#include <errno.h> + +#include "custom_baud.h" + +int set_custom_baudrate(int fd, unsigned int baud, const enum custom_baud_stage stage, void *tio_wanted) { + speed_t speed; + struct termios *wanted = tio_wanted; + + switch (stage) { + case BEFORE_FLAGS: + break; + + case WITH_FLAGS: + if (cfsetspeed(wanted, B19200) == -1) + return -1; + break; + + case AFTER_FLAGS: + speed = baud; + + if (ioctl(fd, IOSSIOSPEED, &speed) == -1) + return -1; + + cfsetispeed(wanted, speed); + cfsetospeed(wanted, speed); + break; + } + + return 0; +} + +int use_custom_baud(unsigned int baud, const struct baudentry *baudtable) +{ + int i; + for (i = 0; baudtable[i].baud; i++) { + if (baudtable[i].baud == baud) + return 0; + + if (baudtable[i].baud > baud) + return 1; + } + return 1; +} diff --git a/meson.build b/meson.build index a93b1f9..8c11e77 100644 --- a/meson.build +++ b/meson.build @@ -108,6 +108,8 @@ if host_machine.system() == 'linux' custom_baud_c = 'custom_baud_linux.c' +elif host_machine.system() == 'darwin' + custom_baud_c = 'custom_baud_darwin.c' else custom_baud_c = 'custom_baud.c' endif diff --git a/serial.c b/serial.c index cfc9b1e..4b4ca39 100644 --- a/serial.c +++ b/serial.c @@ -47,6 +47,10 @@ * On Linux there is a non-standard way to use arbitrary baud rates that we use if there is no * matching standard rate, see custom_baud.c * + * On Darwin there is also a non-standard ioctl() to set arbitrary baud rates + * and any above 230400, see custom_baud_darwin.c and + * https://opensource.apple.com/source/IOSerialFamily/IOSerialFamily-91/tests/… + * * On Windows there exist similar macros (starting with CBR_ instead of B) but they are only defined for * backwards compatibility and the API supports arbitrary baud rates in the same manner as the macros, see * http://msdn.microsoft.com/en-us/library/windows/desktop/aa363214(v=vs.85).a… @@ -71,6 +75,7 @@ #ifdef B230400 BAUDENTRY(230400) #endif +#if !IS_DARWIN #ifdef B460800 BAUDENTRY(460800) #endif @@ -107,6 +112,7 @@ #ifdef B4000000 BAUDENTRY(4000000) #endif +#endif /* !IS_DARWIN */ {0, 0} /* Terminator */ }; -- 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: 1 Gerrit-Owner: Peter Stuge <peter(a)stuge.se> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: Makefile, meson: Add IS_DARWIN analog to IS_WINDOWS
by Peter Stuge (Code Review) 11 Dec '22

11 Dec '22

Attention is currently required from: Thomas Heijligen. Peter Stuge has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70570 ) Change subject: Makefile, meson: Add IS_DARWIN analog to IS_WINDOWS ...................................................................... Makefile, meson: Add IS_DARWIN analog to IS_WINDOWS Change-Id: I44ae7dbcd17079e07e2cea8354325ed45d9310e3 Signed-off-by: Peter Stuge <peter(a)stuge.se> --- M Makefile M meson.build 2 files changed, 21 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/70/70570/1 diff --git a/Makefile b/Makefile index f92cefd..729ff7a 100644 --- a/Makefile +++ b/Makefile @@ -260,6 +260,12 @@ override CFLAGS += -Iinclude +ifeq ($(TARGET_OS), Darwin) +FEATURE_FLAGS += -D'IS_DARWIN=1' +else +FEATURE_FLAGS += -D'IS_DARWIN=0' +endif + ifeq ($(TARGET_OS), DOS) # DJGPP has odd uint*_t definitions which cause lots of format string warnings. override CFLAGS += -Wno-format diff --git a/meson.build b/meson.build index 722822c..a93b1f9 100644 --- a/meson.build +++ b/meson.build @@ -100,6 +100,11 @@ else add_project_arguments('-DIS_WINDOWS=0', language : 'c') endif +if host_machine.system() == 'darwin' + add_project_arguments('-DIS_DARWIN=1', language : 'c') +else + add_project_arguments('-DIS_DARWIN=0', language : 'c') +endif if host_machine.system() == 'linux' custom_baud_c = 'custom_baud_linux.c' -- To view, visit https://review.coreboot.org/c/flashrom/+/70570 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I44ae7dbcd17079e07e2cea8354325ed45d9310e3 Gerrit-Change-Number: 70570 Gerrit-PatchSet: 1 Gerrit-Owner: Peter Stuge <peter(a)stuge.se> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: serial: Call set_custom_baudrate() thrice
by Peter Stuge (Code Review) 11 Dec '22

11 Dec '22

Peter Stuge has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70569 ) Change subject: serial: Call set_custom_baudrate() thrice ...................................................................... serial: Call set_custom_baudrate() thrice Call the function before tcsetattr() settings are known, then again with settings prepared but not yet applied and finally a third time after tcsetattr(). Darwin support needs this change; there custom_baud code must be called to modify the settings passed to tcsetattr() and then again after tcsetattr() returns. The change should be non-functional on all currently supported systems; current code calls set_custom_baudrate() before any tcsetattr() settings are prepared, so we have three stages in total. This change originates from discussion of the macOS patch proposed by Denis Ahrens in https://review.coreboot.org/c/flashrom/+/67822 Change-Id: I40cc443cfb7bf6b212b31826d437b898cc13c427 Signed-off-by: Peter Stuge <peter(a)stuge.se> --- M custom_baud.c M custom_baud_linux.c M include/custom_baud.h M serial.c 4 files changed, 52 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/69/70569/1 diff --git a/custom_baud.c b/custom_baud.c index 28f182c..8bbe6cc 100644 --- a/custom_baud.c +++ b/custom_baud.c @@ -19,7 +19,7 @@ #include "custom_baud.h" /* Stub, should not get called. */ -int set_custom_baudrate(int fd, unsigned int baud) +int set_custom_baudrate(int fd, unsigned int baud, const enum custom_baud_stage stage, void *tio_wanted) { errno = ENOSYS; /* Hoping "Function not supported" will make you look here. */ return -1; diff --git a/custom_baud_linux.c b/custom_baud_linux.c index 2d5f261..761d496 100644 --- a/custom_baud_linux.c +++ b/custom_baud_linux.c @@ -29,9 +29,13 @@ * for more info. */ -int set_custom_baudrate(int fd, unsigned int baud) +int set_custom_baudrate(int fd, unsigned int baud, const enum custom_baud_stage stage, void *tio_wanted) { struct termios2 tio; + + if (stage != BEFORE_FLAGS) + return 0; + if (ioctl(fd, TCGETS2, &tio)) { return -1; } diff --git a/include/custom_baud.h b/include/custom_baud.h index c8b8fc2..38e6cfc 100644 --- a/include/custom_baud.h +++ b/include/custom_baud.h @@ -22,7 +22,13 @@ unsigned int baud; }; -int set_custom_baudrate(int fd, unsigned int baud); +enum custom_baud_stage { + BEFORE_FLAGS = 0, + WITH_FLAGS, + AFTER_FLAGS +}; + +int set_custom_baudrate(int fd, unsigned int baud, const enum custom_baud_stage stage, void *tio_wanted); /* Returns 1 if non-exact rate would be used, and setting a custom rate is supported. The baudtable must be in ascending order and terminated with a 0-baud entry. */ diff --git a/serial.c b/serial.c index 72f9ef6..cfc9b1e 100644 --- a/serial.c +++ b/serial.c @@ -179,6 +179,7 @@ } msg_pdbg("Baud rate is %ld.\n", dcb.BaudRate); #else + int custom_baud = (baud >= 0 && use_custom_baud(baud, sp_baudtable)); struct termios wanted, observed; if (tcgetattr(fd, &observed) != 0) { msg_perr_strerror("Could not fetch original serial port configuration: "); @@ -186,8 +187,8 @@ } wanted = observed; if (baud >= 0) { - if (use_custom_baud(baud, sp_baudtable)) { - if (set_custom_baudrate(fd, baud)) { + if (custom_baud) { + if (set_custom_baudrate(fd, baud, BEFORE_FLAGS, NULL)) { msg_perr_strerror("Could not set custom baudrate: "); return 1; } @@ -198,7 +199,6 @@ msg_perr_strerror("Could not fetch serial port configuration: "); return 1; } - msg_pdbg("Using custom baud rate.\n"); } else { const struct baudentry *entry = round_baud(baud); if (cfsetispeed(&wanted, entry->flag) != 0 || cfsetospeed(&wanted, entry->flag) != 0) { @@ -212,6 +212,10 @@ wanted.c_lflag &= ~(ICANON | ECHO | ECHOE | ISIG | IEXTEN); wanted.c_iflag &= ~(IXON | IXOFF | IXANY | ICRNL | IGNCR | INLCR); wanted.c_oflag &= ~OPOST; + if (custom_baud && set_custom_baudrate(fd, baud, WITH_FLAGS, &wanted)) { + msg_perr_strerror("Could not set custom baudrate: "); + return 1; + } if (tcsetattr(fd, TCSANOW, &wanted) != 0) { msg_perr_strerror("Could not change serial port configuration: "); return 1; @@ -236,6 +240,13 @@ (long)observed.c_oflag, (long)wanted.c_oflag ); } + if (custom_baud) { + if (set_custom_baudrate(fd, baud, AFTER_FLAGS, &wanted)) { + msg_perr_strerror("Could not set custom baudrate: "); + return 1; + } + msg_pdbg("Using custom baud rate.\n"); + } if (cfgetispeed(&observed) != cfgetispeed(&wanted) || cfgetospeed(&observed) != cfgetospeed(&wanted)) { msg_pwarn("Could not set baud rates exactly.\n"); -- To view, visit https://review.coreboot.org/c/flashrom/+/70569 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I40cc443cfb7bf6b212b31826d437b898cc13c427 Gerrit-Change-Number: 70569 Gerrit-PatchSet: 1 Gerrit-Owner: Peter Stuge <peter(a)stuge.se> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: meson: Determine custom_baud source file only once
by Peter Stuge (Code Review) 11 Dec '22

11 Dec '22

Peter Stuge has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70568 ) Change subject: meson: Determine custom_baud source file only once ...................................................................... meson: Determine custom_baud source file only once Change-Id: I13221bdca7d14a483f416e81e3830a495659a85e Signed-off-by: Peter Stuge <peter(a)stuge.se> --- M meson.build 1 file changed, 19 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/68/70568/1 diff --git a/meson.build b/meson.build index 56ab55c..722822c 100644 --- a/meson.build +++ b/meson.build @@ -101,6 +101,12 @@ add_project_arguments('-DIS_WINDOWS=0', language : 'c') endif +if host_machine.system() == 'linux' + custom_baud_c = 'custom_baud_linux.c' +else + custom_baud_c = 'custom_baud.c' +endif + systems_hwaccess = [ 'linux', 'openbsd', 'freebsd', 'dragonfly', 'netbsd' ] systems_serial = [ 'linux', 'openbsd', 'freebsd', 'dragonfly', 'netbsd', 'darwin' ] @@ -175,7 +181,7 @@ 'buspirate_spi' : { 'systems' : systems_serial, 'groups' : [ group_serial, group_external ], - 'srcs' : files('buspirate_spi.c', 'serial.c', (host_machine.system() == 'linux' ? 'custom_baud_linux.c' : 'custom_baud.c')), + 'srcs' : files('buspirate_spi.c', 'serial.c', custom_baud_c), 'flags' : [ '-DCONFIG_BUSPIRATE_SPI=1' ], }, 'ch341a_spi' : { @@ -374,7 +380,7 @@ 'pony_spi' : { 'systems' : systems_serial, 'groups' : [ group_serial, group_external ], - 'srcs' : files('pony_spi.c', 'serial.c', (host_machine.system() == 'linux' ? 'custom_baud_linux.c' : 'custom_baud.c')), + 'srcs' : files('pony_spi.c', 'serial.c', custom_baud_c), 'flags' : [ '-DCONFIG_PONY_SPI=1' ], }, 'raiden_debug_spi' : { @@ -416,7 +422,7 @@ 'serprog' : { 'systems' : systems_serial, 'groups' : [ group_serial, group_external ], - 'srcs' : files('serprog.c', 'serial.c', (host_machine.system() == 'linux' ? 'custom_baud_linux.c' : 'custom_baud.c')), + 'srcs' : files('serprog.c', 'serial.c', custom_baud_c), 'flags' : [ '-DCONFIG_SERPROG=1' ], }, 'stlinkv3_spi' : { -- To view, visit https://review.coreboot.org/c/flashrom/+/70568 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I13221bdca7d14a483f416e81e3830a495659a85e Gerrit-Change-Number: 70568 Gerrit-PatchSet: 1 Gerrit-Owner: Peter Stuge <peter(a)stuge.se> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: flashrom: Check for flash access restricitons in verify_range()
by Edward O'Callaghan (Code Review) 10 Dec '22

10 Dec '22

Attention is currently required from: Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70515 ) Change subject: flashrom: Check for flash access restricitons in verify_range() ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70515 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I61dfadd3c75365f2e55abeea75f673ab791ca5cc Gerrit-Change-Number: 70515 Gerrit-PatchSet: 3 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sat, 10 Dec 2022 23:49:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2022