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

Change in flashrom[master]: dediprog: Enable 4BA for all protocols
by Patrick Rudolph (Code Review) 11 May '21

11 May '21

Hello Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/44776 to review the following change. Change subject: dediprog: Enable 4BA for all protocols ...................................................................... dediprog: Enable 4BA for all protocols * Tested on SF600 with protocol version 3 with "W25Q256.W" (32768 kB) * Tested on SF100 with protocol version 2 with "W25Q256.W" (32768 kB) As it works with all protocol version, assume that the SF200 works fine as well. Change-Id: Iee0ba972245b9317ef86345432fec5fc32614888 Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com> --- M dediprog.c 1 file changed, 1 insertion(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/44776/1 diff --git a/dediprog.c b/dediprog.c index 827d5e4..13365cd 100644 --- a/dediprog.c +++ b/dediprog.c @@ -993,7 +993,7 @@ } static struct spi_master spi_master_dediprog = { - .features = SPI_MASTER_NO_4BA_MODES, + .features = SPI_MASTER_4BA, .max_data_read = 16, /* 18 seems to work fine as well, but 19 times out sometimes with FW 5.15. */ .max_data_write = 16, .command = dediprog_spi_send_command, @@ -1268,12 +1268,6 @@ if (dediprog_standalone_mode()) return 1; - if (dediprog_devicetype == DEV_SF100 && protocol() == PROTOCOL_V1) - spi_master_dediprog.features &= ~SPI_MASTER_NO_4BA_MODES; - - if (protocol() == PROTOCOL_V2) - spi_master_dediprog.features |= SPI_MASTER_4BA; - if (register_spi_master(&spi_master_dediprog) || dediprog_set_leds(LED_NONE)) return 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/44776 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iee0ba972245b9317ef86345432fec5fc32614888 Gerrit-Change-Number: 44776 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

3 3

Change in flashrom[master]: tests: Start port-i/o mocking framework
by Anastasia Klimchuk (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Nico Huber. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52794 ) Change subject: tests: Start port-i/o mocking framework ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/52794 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8df02832deba80761b57435244a29d0d9b4e2649 Gerrit-Change-Number: 52794 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Fri, 30 Apr 2021 22:19:55 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
by Simon Glass (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c ...................................................................... Patch Set 10: (3 comments) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/51487/comment/410e9962_03cf28e0 PS10, Line 46: struct mec1308_test_data *test_data = current_io->test_data; I think you should add a blank line between decls and code https://review.coreboot.org/c/flashrom/+/51487/comment/3cbd502a_6226b9c7 PS10, Line 53: MEC1308_SIO_PORT1 Can you not access these values somehow, or are you trying to test that the enums are correct? File tests/tests.c: https://review.coreboot.org/c/flashrom/+/51487/comment/00779891_f4169476 PS10, Line 92: if (current_io && current_io->outb) Why are these allowed to be NULL? -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 10 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Simon Glass <sjg(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 30 Apr 2021 15:59:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] tests: Abstract port I/O and factor mec1308 specifics out
by Nico Huber (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52604 ) Change subject: [RFC] tests: Abstract port I/O and factor mec1308 specifics out ...................................................................... Patch Set 1: (1 comment) File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/52604/comment/2e6845de_2d7fe86b PS1, Line 1: #ifndef _IO_MOCK_H_ : #define _IO_MOCK_H_ > I just realised, this is a new file and it doesn't have Copyright header. […] Um, I don't think this is copyrightable. But arguing about it is probably more work than doing the patch. :D CB:52794 -- To view, visit https://review.coreboot.org/c/flashrom/+/52604 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia3a424478456a7c968b3c736ba371d755c494f30 Gerrit-Change-Number: 52604 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 30 Apr 2021 14:53:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Start port-i/o mocking framework
by Angel Pons (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52794 ) Change subject: tests: Start port-i/o mocking framework ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/52794 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8df02832deba80761b57435244a29d0d9b4e2649 Gerrit-Change-Number: 52794 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Fri, 30 Apr 2021 14:53:33 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Start port-i/o mocking framework
by Nico Huber (Code Review) 30 Apr '21

30 Apr '21

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52794 ) Change subject: tests: Start port-i/o mocking framework ...................................................................... tests: Start port-i/o mocking framework This will be used to mock the i/o needs of indvidiual programmer drivers. For each programmer driver, a `struct io_mock` can be registered to dispatch inb()/outb() and friends. Change-Id: I8df02832deba80761b57435244a29d0d9b4e2649 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- A tests/io_mock.h 1 file changed, 49 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/94/52794/1 diff --git a/tests/io_mock.h b/tests/io_mock.h new file mode 100644 index 0000000..056e6bf --- /dev/null +++ b/tests/io_mock.h @@ -0,0 +1,49 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (c) 2021 Nico Huber <nico.h(a)gmx.de> + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. Neither the name of the author nor the names of its contributors + * may be used to endorse or promote products derived from this + * software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#ifndef _IO_MOCK_H_ +#define _IO_MOCK_H_ + +struct io_mock { + void *priv; + + void (*outb)(void *priv, unsigned char value, unsigned short port); + unsigned char (*inb)(void *priv, unsigned short port); + + void (*outw)(void *priv, unsigned short value, unsigned short port); + unsigned short (*inw)(void *priv, unsigned short port); + + void (*outl)(void *priv, unsigned int value, unsigned short port); + unsigned int (*inl)(void *priv, unsigned short port); +}; + +void io_mock_register(const struct io_mock *io); + +#endif -- To view, visit https://review.coreboot.org/c/flashrom/+/52794 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8df02832deba80761b57435244a29d0d9b4e2649 Gerrit-Change-Number: 52794 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: pickit2_spi.c: Refactor singleton states into reentrant pattern
by Edward O'Callaghan (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52774 ) Change subject: pickit2_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: (1 comment) File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/52774/comment/57117c56_466cc8fd PS1, Line 343: static struct spi_master spi_master_pickit2 = { > This will bug me with every single patch. I think the API is just broken […] Same and this is something Anastasia and I actively discussed. I suggested to Anastasia to focus on getting the easier boilerplate-like patches such as this one front loaded before moving forwards with the API change so that it is well informed, primarily by building heuristic. There were a few edge cases Anastasia identified upon reflection and so we wanted to bring the tree into a more consistent state. Specifically, two main things: i) get all the drivers to be reentrant by leveraging data in a consistent manner, ii) untangle the purpose of shutdown such that its purpose singular in its duties. One other dimension suggested was that the testing efforts running in tandem to this should be easier to write and have larger cov once drivers follow a more consistent pattern. The data field was a great introduction however was never fully realised so we agreed this would be a good milestone to achieve. I'll let Anastasia talk to the precise details however I wanted to chime in here Nico so you perhaps get some visibility into the strategy set out to help Anastasia make consistent progress. -- To view, visit https://review.coreboot.org/c/flashrom/+/52774 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibacc4738bee02c371c41583d321e0337128ad18a Gerrit-Change-Number: 52774 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 30 Apr 2021 12:30:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: pickit2_spi.c: Refactor singleton states into reentrant pattern
by Nico Huber (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52774 ) Change subject: pickit2_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: (3 comments) File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/52774/comment/39a5cbe4_1e92716f PS1, Line 57: }; Do we need this struct? I guess it would make it easier to extend in the future, but this seems hard to predict. We could also pass the libusb handle directly as `data`. https://review.coreboot.org/c/flashrom/+/52774/comment/e1d4b973_7f01382a PS1, Line 343: static struct spi_master spi_master_pickit2 = { This will bug me with every single patch. I think the API is just broken and maybe we should fix it before making more use of it. The per-instance `data` pointer should simply be passed to register_spi_master(), IMHO. This might actually be easier to address right now as there are not that many programmer drivers that use the `data` pointer yet. https://review.coreboot.org/c/flashrom/+/52774/comment/0b21cbcc_982ab4fb PS1, Line 472: pickit2_data = calloc(1, sizeof(*pickit2_data)); No NULL check? (Could be avoided by not using a struct.) -- To view, visit https://review.coreboot.org/c/flashrom/+/52774 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibacc4738bee02c371c41583d321e0337128ad18a Gerrit-Change-Number: 52774 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 30 Apr 2021 11:39:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: pickit2_spi.c: Separate shutdown from failed init cleanup
by Nico Huber (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52773 ) Change subject: pickit2_spi.c: Separate shutdown from failed init cleanup ...................................................................... Patch Set 1: Code-Review+2 (1 comment) File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/52773/comment/deb669b6_f2664fc6 PS1, Line 482: goto init_err_cleanup_exit; Just one comment to make this review not too boring: This actually changes behavior. The original version failed to call pickit2_shutdown() if its registration failed. It's a minor thing in this case (why would registration fail?). Generally it's prefered though to focus on the functional changes in the commit message (or just have them in a separate change). -- To view, visit https://review.coreboot.org/c/flashrom/+/52773 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1b672b33169a7a1b6ceab190ad3f48c2f35c3a1f Gerrit-Change-Number: 52773 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 30 Apr 2021 11:22:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] tests: Abstract port I/O and factor mec1308 specifics out
by Anastasia Klimchuk (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Nico Huber. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52604 ) Change subject: [RFC] tests: Abstract port I/O and factor mec1308 specifics out ...................................................................... Patch Set 1: (1 comment) File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/52604/comment/d117d6c2_bc06a158 PS1, Line 1: #ifndef _IO_MOCK_H_ : #define _IO_MOCK_H_ I just realised, this is a new file and it doesn't have Copyright header. Would you like to create a patch with this file and submit? I did little modifications for 51487, but I can rebase 51487 on the top and add my modifications. What do you think? -- To view, visit https://review.coreboot.org/c/flashrom/+/52604 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia3a424478456a7c968b3c736ba371d755c494f30 Gerrit-Change-Number: 52604 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Fri, 30 Apr 2021 09:20:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2021