Attention is currently required from: Thomas Heijligen, Angel Pons, qianfan, Anastasia Klimchuk.
Nicholas Chin has removed Felix Singer from this change. ( https://review.coreboot.org/c/flashrom/+/70573 )
Change subject: ch347_spi.c: Add initial support for the WCH CH347
......................................................................
Removed reviewer Felix Singer <felixsinger(a)posteo.net>.
--
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: 4
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: 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-CC: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-CC: qianfan <qianfanguijin(a)163.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: qianfan <qianfanguijin(a)163.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: deleteReviewer
Attention is currently required from: Felix Singer, Thomas Heijligen, Angel Pons, qianfan, 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 4:
(11 comments)
Patchset:
PS3:
> After a number of discussions in several irc channels, the conclusion is: this patch is going ahead […]
I believe I've done all those things now. Let me know if I'm still missing anything
File ch347_spi.c:
https://review.coreboot.org/c/flashrom/+/70573/comment/93507707_1148e153
PS3, Line 62: /* TODO: Set this depending on the mode */
> new line before
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/6d23e2c2_4b5fd952
PS3, Line 68: ch347_data->handle = NULL;
> new line before
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/e5cc41ef_ff0f8c97
PS3, Line 69: free(data);
> Yes please, free the data :) […]
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/5cca6826_fb595c44
PS3, Line 106: uint8_t *buffer = malloc(packet_len);
> +1
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/e48f5f4f_6d7a846c
PS3, Line 123: Could not read write response
> It is confusing, given that both "read" and "write" are the operations, what does it mean "Could not […]
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/7b37ff3f_c0d40a97
PS3, Line 145: malloc(CH347_MAX_DATA_LEN + 3);
> Same as above
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/f26aef5e_6dd69049
PS3, Line 149: 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;
> Use an initialiser as done in `ch347_set_cs()`?
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/75fcdd49_4180b609
PS3, Line 223: -3
> nit: spacing here and in the line below
Done
https://review.coreboot.org/c/flashrom/+/70573/comment/b568f0e4_1aabbdec
PS3, Line 222: 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;
> Use an initialiser as done in `ch347_set_cs()`?
Done. I had initially done it like this as I was previously using `buff` to read the original SPI configuration and then overwriting it with these values, as I thought that was a necessary step in order for the SPI_CFG command to respond. Seems like reading the initial config isn't actually necessary so I removed the initial read and initialized `buff` with the config.
https://review.coreboot.org/c/flashrom/+/70573/comment/2b8fb497_70d08469
PS3, Line 261: 64 * 1024
> I was reading over some of Thomas's comments in CB:72057 (Patchset 19, lines 120 and 121) and it see […]
Done
--
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: 4
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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: 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-CC: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-CC: qianfan <qianfanguijin(a)163.com>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: qianfan <qianfanguijin(a)163.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 20 Feb 2023 03:40:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Angel Pons, qianfan.
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 (#4).
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 MAINTAINERS
M Makefile
A ch347_spi.c
M flashrom.8.tmpl
M include/programmer.h
M meson.build
M meson_options.txt
M programmer_table.c
M test_build.sh
9 files changed, 410 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/73/70573/4
--
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: 4
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: 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-CC: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-CC: qianfan <qianfanguijin(a)163.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: qianfan <qianfanguijin(a)163.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Thomas Heijligen, Edward O'Callaghan.
Hello build bot (Jenkins), Thomas Heijligen, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/72057
to look at the new patch set (#22).
Change subject: Add support for VIA VL805 USB controller flashing
......................................................................
Add support for VIA VL805 USB controller flashing
It works fine for me on my Raspberry Pi 4 Model B. Was able read read,
erase, write, verify. Only thing is the WP with the W25X10 gives a
untested warning.
Change-Id: I71435afcacdf97e14d627e35bce3d29de9657f38
Signed-off-by: Christopher Lentocha <christopherericlentocha(a)gmail.com>
---
M Makefile
M flashrom.8.tmpl
M include/programmer.h
M meson.build
M meson_options.txt
M programmer_table.c
A vl805.c
7 files changed, 212 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/57/72057/22
--
To view, visit https://review.coreboot.org/c/flashrom/+/72057
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I71435afcacdf97e14d627e35bce3d29de9657f38
Gerrit-Change-Number: 72057
Gerrit-PatchSet: 22
Gerrit-Owner: ChrisEric1 CECL <christopherericlentocha(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: ChrisEric1 CECL, Thomas Heijligen.
Hello build bot (Jenkins), Thomas Heijligen, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/72057
to look at the new patch set (#21).
Change subject: Add support for VIA VL805 USB Controller flashing
......................................................................
Add support for VIA VL805 USB Controller flashing
It works fine for me on my Raspberry Pi 4 Model B. Was able read read,
erase, write, verify. Only thing is the WP with the W25X10 gives a
untested warning.
Change-Id: I71435afcacdf97e14d627e35bce3d29de9657f38
Signed-off-by: Christopher Lentocha <christopherericlentocha(a)gmail.com>
---
M Makefile
M flashrom.8.tmpl
M include/programmer.h
M meson.build
M meson_options.txt
M programmer_table.c
A vl805.c
7 files changed, 212 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/57/72057/21
--
To view, visit https://review.coreboot.org/c/flashrom/+/72057
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I71435afcacdf97e14d627e35bce3d29de9657f38
Gerrit-Change-Number: 72057
Gerrit-PatchSet: 21
Gerrit-Owner: ChrisEric1 CECL <christopherericlentocha(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Attention: ChrisEric1 CECL <christopherericlentocha(a)gmail.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-MessageType: newpatchset
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/72966 )
Change subject: cli_classic.c: Drop spurious cast
......................................................................
cli_classic.c: Drop spurious cast
This cast should not be required.
Change-Id: Ia3a658dd6f4986eb6da84a11bce66f53e1571469
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/72966
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Sam McNally <sammc(a)google.com>
---
M cli_classic.c
1 file changed, 16 insertions(+), 2 deletions(-)
Approvals:
build bot (Jenkins): Verified
Sam McNally: Looks good to me, approved
diff --git a/cli_classic.c b/cli_classic.c
index c72836f..da68919 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -662,8 +662,7 @@
* chip when a flash device gets opened with fd 1 or 2.
*/
if (check_file(stdout) && check_file(stderr)) {
- flashrom_set_log_callback(
- (flashrom_log_callback *)&flashrom_print_cb);
+ flashrom_set_log_callback(&flashrom_print_cb);
}
print_version();
--
To view, visit https://review.coreboot.org/c/flashrom/+/72966
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia3a658dd6f4986eb6da84a11bce66f53e1571469
Gerrit-Change-Number: 72966
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(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-MessageType: merged