December 2022 - flashrom-gerrit

[S] Change in flashrom[master]: ch347t_spi.c: Move ch347_device struct to flashctx
by Nicholas Chin (Code Review) 19 Dec '22

19 Dec '22

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71066 ) Change subject: ch347t_spi.c: Move ch347_device struct to flashctx ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I initially uploaded CB:70573 without realizing that CB:70429 had previously been uploaded by qianfan Zhao ("ch347" didn't turn up any results in Gerrit as Qianfan had used "ch347t", turns out you need to use a regex search to get a partial match of a word). This commit aims to take the major unique features of my original patch and rebase them on top of Qianfan's port. -- To view, visit https://review.coreboot.org/c/flashrom/+/71066 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If3ff96dedd2b591200dd69f54df753668853beca Gerrit-Change-Number: 71066 Gerrit-PatchSet: 1 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 19 Dec 2022 18:43:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ch347t_spi.c: Move ch347_device struct to flashctx
by Nicholas Chin (Code Review) 19 Dec '22

19 Dec '22

Nicholas Chin has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/71066 ) Change subject: ch347t_spi.c: Move ch347_device struct to flashctx ...................................................................... ch347t_spi.c: Move ch347_device struct to flashctx Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com> Change-Id: If3ff96dedd2b591200dd69f54df753668853beca --- M ch347t_spi.c 1 file changed, 18 insertions(+), 14 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/66/71066/1 diff --git a/ch347t_spi.c b/ch347t_spi.c index 87c9bb4..45ac1df 100644 --- a/ch347t_spi.c +++ b/ch347t_spi.c @@ -2,6 +2,7 @@ * This file is part of the flashrom project. * * Copyright (C) 2022 qianfan Zhao <qianfanguijin(a)163.com> + * 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 @@ -15,6 +16,7 @@ */ #include <string.h> +#include <stdlib.h> #include <libusb.h> #include "flash.h" #include "platform.h" @@ -318,10 +320,6 @@ return 0; } -static struct ch347_device ch347_device = { - .handle = NULL, -}; - static const struct dev_entry devs_ch347t_spi[] = { {0x1A86, 0x55DB, OK, "Winchiphead (WCH)", "CH347T"}, {0}, @@ -329,7 +327,7 @@ static int ch347t_spi_spi_send_command(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) { - struct ch347_device *dev = &ch347_device; + struct ch347_device *dev = flash->mst->spi.data; int ret; if (!dev->handle) @@ -358,7 +356,7 @@ static int ch347t_spi_shutdown(void *data) { - struct ch347_device *dev = &ch347_device; + struct ch347_device *dev = data; struct libusb_device_handle *handle = dev->handle; if (!handle) @@ -369,6 +367,7 @@ libusb_close(handle); libusb_exit(NULL); dev->handle = NULL; + free(data); return 0; } @@ -387,15 +386,10 @@ static int ch347t_spi_init(const struct programmer_cfg *cfg) { - struct ch347_device *dev = &ch347_device; - struct libusb_device_handle *handle; + struct ch347_device *dev = calloc(1, sizeof(*dev)); + struct libusb_device_handle *handle = NULL; int ret; - if (dev->handle != NULL) { - msg_cerr("%s: handle already set! Please report a bug at flashrom(a)flashrom.org\n", __func__); - return -1; - } - ret = libusb_init(NULL); if (ret < 0) { msg_perr("Couldn't initialize libusb!\n"); @@ -437,7 +431,7 @@ goto release_interface; } - return register_spi_master(&spi_master_ch347t_spi, NULL); + return register_spi_master(&spi_master_ch347t_spi, dev); release_interface: libusb_release_interface(handle, 0); -- To view, visit https://review.coreboot.org/c/flashrom/+/71066 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If3ff96dedd2b591200dd69f54df753668853beca Gerrit-Change-Number: 71066 Gerrit-PatchSet: 1 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-MessageType: newchange

1 0

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

19 Dec '22

Attention is currently required from: 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 3: (1 comment) Patchset: PS3: > @Nicholas Could you please test PR #70529 on yours board and can I merge your's PR? I can add your's […] I've added some comments on your patch. I think it would probably be easiest if I make a new patch that's rebased on top of your's instead of merging the code into a single patch -- 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-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-Comment-Date: Mon, 19 Dec 2022 18:09:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: qianfan <qianfanguijin(a)163.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

1 0

[L] Change in flashrom[master]: Add initial CH347T SPI programmer
by Nicholas Chin (Code Review) 19 Dec '22

19 Dec '22

Attention is currently required from: qianfan, Angel Pons. Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70529 ) Change subject: Add initial CH347T SPI programmer ...................................................................... Patch Set 3: (11 comments) Commit Message: PS3: It would probably be good to specify that this only supports mode 1 (vendor protocol) right now and not mode 2 (HID protocol). It would also be good to specify what settings are currently hard coded, such as the clock speed, spi mode, and that this uses CS0. File ch347t_spi.c: https://review.coreboot.org/c/flashrom/+/70529/comment/bd27da55_992ba351 PS2, Line 34: CH347_CMD_SPI_CONTROL, Maybe make it more clear that this controls the CS lines? File ch347t_spi.c: https://review.coreboot.org/c/flashrom/+/70529/comment/29aaace6_bf9ff960 PS3, Line 30: #define CH347_MAX_DATA_READ 4096 This probably could be set to a higher value. I've tried it with a max read size of 64KiB (64*1024) and it saves about a second with a 16MiB flash chip. The read command takes in a 32 bit integer to technically this could be 4GiB, though that would be impractical due to memory constraints. https://review.coreboot.org/c/flashrom/+/70529/comment/e5f39547_68b349d0 PS3, Line 41: enum spi_prescaler { : SPI_BAUDRATEPRESCALER_2 = 0x00, : SPI_BAUDRATEPRESCALER_4 = 0x08, : SPI_BAUDRATEPRESCALER_8 = 0x10, : SPI_BAUDRATEPRESCALER_16 = 0x18, : SPI_BAUDRATEPRESCALER_32 = 0x20, : SPI_BAUDRATEPRESCALER_64 = 0x28, : SPI_BAUDRATEPRESCALER_128 = 0x30, : SPI_BAUDRATEPRESCALER_256 = 0x38 : }; Alternatively the prescaler values could start from 1 and then the formula would be freq = 60 MHz / prescaler https://review.coreboot.org/c/flashrom/+/70529/comment/21d6553b_4d1f6fbc PS3, Line 84: enum spi_nss { : SPI_NSS_SOFT = 0x0200, : SPI_NSS_HARD = 0x0000 : }; I would rename these to be clearer what it does, maybe `enum spi_cs`, `SPI_CS_SOFTWARE`, and `SPI_CS_HARDWARE`. https://review.coreboot.org/c/flashrom/+/70529/comment/77fa3553_9f7465e0 PS3, Line 179: buf, n + 3, This may go over the 512 byte max packet size of the write endpoint since n can be up to 512 due to the previous code. That said, I have seen the vendor library attempt to send over 700 bytes in one packet... https://review.coreboot.org/c/flashrom/+/70529/comment/c3d94cd2_683d8bfc PS3, Line 330: ch347t_spi_spi_send_command The two "spi"s are redundant https://review.coreboot.org/c/flashrom/+/70529/comment/6ca21b37_d0fb5d29 PS3, Line 367: libusb_release_interface(handle, 0); : libusb_attach_kernel_driver(handle, 0); Interface 2, see comment in `ch347t_spi_init` https://review.coreboot.org/c/flashrom/+/70529/comment/93dcfb48_99039daa PS3, Line 420: 0 Mode 1 uses interface 2, and the programmer currently fails if the vendor kernel driver is loaded. https://review.coreboot.org/c/flashrom/+/70529/comment/246a7deb_2b375514 PS3, Line 425: 0 Interface 2 https://review.coreboot.org/c/flashrom/+/70529/comment/caadb730_61eeb329 PS3, Line 442: release_interface: : libusb_release_interface(handle, 0); : close_handle: : libusb_attach_kernel_driver(handle, 0); Interface 2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70529 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6da5e308af76693e60308c8165349128e517e09a Gerrit-Change-Number: 70529 Gerrit-PatchSet: 3 Gerrit-Owner: qianfan <qianfanguijin(a)163.com> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.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-Attention: qianfan <qianfanguijin(a)163.com> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com> Gerrit-Comment-Date: Mon, 19 Dec 2022 18:07:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

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

19 Dec '22

Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk, Nicholas Chin. qianfan 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: (1 comment) Patchset: PS3: > Nicholas, Qianfan, thank you so much for your contribution! It doesn't happen often that we get a ne […] @Nicholas Could you please test PR #70529 on yours board and can I merge your's PR? I can add your's signed-off-by and copyright on my PR and let's develop this togger. Be a MAINTAINERS is very cool and I very do my best if I can. PS: #70529 is have not reviewed, can we review it first? -- 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-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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Comment-Date: Mon, 19 Dec 2022 09:49:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: qianfan <qianfanguijin(a)163.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

1 0

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

18 Dec '22

Attention is currently required from: Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70517 ) Change subject: flashrom: Check for flash access restricitons in erase path ...................................................................... Patch Set 7: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/70517 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If027a96a024782c7707c6d38680709a1a117f3ef Gerrit-Change-Number: 70517 Gerrit-PatchSet: 7 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 18 Dec 2022 12:30:13 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: layout: layout_next_included_region() return a region not romentry
by Edward O'Callaghan (Code Review) 18 Dec '22

18 Dec '22

Attention is currently required from: Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70892 ) Change subject: layout: layout_next_included_region() return a region not romentry ...................................................................... Patch Set 1: (1 comment) File layout.c: https://review.coreboot.org/c/flashrom/+/70892/comment/525297c6_26d2c5cf PS1, Line 374: lowest I guess `lowest` could be NULL? -- To view, visit https://review.coreboot.org/c/flashrom/+/70892 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibeab87c87b9da3112e1866bc058e0f85bacddc7a Gerrit-Change-Number: 70892 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 18 Dec 2022 12:28:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom: Check for flash access restricitons in write_flash()
by Angel Pons (Code Review) 18 Dec '22

18 Dec '22

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70516 ) Change subject: flashrom: Check for flash access restricitons in write_flash() ...................................................................... Patch Set 7: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70516 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idacf0d5218da9d9929f4877fc7665fe608b87fe0 Gerrit-Change-Number: 70516 Gerrit-PatchSet: 7 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 18 Dec 2022 09:39:34 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

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

18 Dec '22

Attention is currently required from: Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70516 ) Change subject: flashrom: Check for flash access restricitons in write_flash() ...................................................................... Patch Set 7: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/70516 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idacf0d5218da9d9929f4877fc7665fe608b87fe0 Gerrit-Change-Number: 70516 Gerrit-PatchSet: 7 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 18 Dec 2022 04:34:53 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

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

18 Dec '22

Attention is currently required from: Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70516 ) Change subject: flashrom: Check for flash access restricitons in write_flash() ...................................................................... Patch Set 6: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70516/comment/3f80dff2_7b0cd287 PS6, Line 10: UNREADABLE > `UNWRITABLE` Done -- To view, visit https://review.coreboot.org/c/flashrom/+/70516 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idacf0d5218da9d9929f4877fc7665fe608b87fe0 Gerrit-Change-Number: 70516 Gerrit-PatchSet: 6 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 18 Dec 2022 01:26:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2022