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

Change in flashrom[master]: hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55741 ) Change subject: hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55741 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3f2f0408be7c00f954b899031b52b2b97ef19ca3 Gerrit-Change-Number: 55741 Gerrit-PatchSet: 6 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 25 Jun 2021 13:17:34 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Do not run a test if its driver is not built
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55295 ) Change subject: tests: Do not run a test if its driver is not built ...................................................................... tests: Do not run a test if its driver is not built For all tests that exist as of today, drivers are built by default, however config options can be disabled and in that case test should not be run. Technically, this is done by skipping the test. BUG=b:181803212 TEST=1) Tested by adding into tests/meson.build -DCONFIG_xxx=0 4 times (for every driver with test), and then running ninja test Result: corresponding test is skipped, all other tests are passed 2) Running ninja test with default config settings (everything is enabled, no overriding in test meson). Result: all tests are passed. 3) Replacing one of config options in the patch with CONFIG_JLINK_SPI which is disabled by default. Result: corresponding test is skipped. Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55295 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M tests/init_shutdown.c 1 file changed, 16 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/tests/init_shutdown.c b/tests/init_shutdown.c index 80440bd..48448f8 100644 --- a/tests/init_shutdown.c +++ b/tests/init_shutdown.c @@ -34,7 +34,11 @@ void dummy_init_and_shutdown_test_success(void **state) { +#if CONFIG_DUMMY == 1 run_lifecycle(state, &programmer_dummy, "bus=parallel+lpc+fwh+spi"); +#else + skip(); +#endif } struct mec1308_io_state { @@ -62,6 +66,7 @@ void mec1308_init_and_shutdown_test_success(void **state) { +#if CONFIG_MEC1308 == 1 struct mec1308_io_state mec1308_io_state = { 0 }; const struct io_mock mec1308_io = { .state = &mec1308_io_state, @@ -75,6 +80,9 @@ run_lifecycle(state, &programmer_mec1308, ""); io_mock_register(NULL); +#else + skip(); +#endif } struct ene_lpc_io_state { @@ -118,6 +126,7 @@ void ene_lpc_init_and_shutdown_test_success(void **state) { +#if CONFIG_ENE_LPC == 1 /* * Current implementation tests for chip ENE_KB932. * Another chip which is not tested here is ENE_KB94X. @@ -134,6 +143,9 @@ run_lifecycle(state, &programmer_ene_lpc, ""); io_mock_register(NULL); +#else + skip(); +#endif } void linux_spi_init_and_shutdown_test_success(void **state) @@ -144,5 +156,9 @@ * and the fallback to getpagesize(). This test does the latter (fallback to * getpagesize). */ +#if CONFIG_LINUX_SPI == 1 run_lifecycle(state, &programmer_linux_spi, "dev=/dev/null"); +#else + skip(); +#endif } -- To view, visit https://review.coreboot.org/c/flashrom/+/55295 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77 Gerrit-Change-Number: 55295 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: 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: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Angel Pons, Michael Niewöhner. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 9: (1 comment) Patchset: PS4: > > Forcing to run the programmer code on an unknown board would be new […] All other EC drivers were added lately without proper review. That they skip sanity checks (and documentation too), is the foremost reason why I'm currently holding a release back. The situation of the superio probing is bad indeed, but not as bad as starting with EC RAM writes. The comment in probe_superio() proves the point. IMHO, we should only probe for expected chips, `board_enable` provides the infrastructure for it. Patches (also to clean up the superio situation) are welcome of course. -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 9 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Fri, 25 Jun 2021 13:14:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: acpi_ec: Implement basic ACPI embedded controller API
by Michael Niewöhner (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Michał Żygowski, Angel Pons. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55714 ) Change subject: acpi_ec: Implement basic ACPI embedded controller API ...................................................................... Patch Set 4: (1 comment) File acpi_ec.c: https://review.coreboot.org/c/flashrom/+/55714/comment/851fde2a_a4647fe6 PS4, Line 23: ontrol_port cmd_port -- To view, visit https://review.coreboot.org/c/flashrom/+/55714 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie3bae8d81c80ae2f286b619e974869e3f2f4545d Gerrit-Change-Number: 55714 Gerrit-PatchSet: 4 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 25 Jun 2021 13:12:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi: Revise error message
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55697 ) Change subject: ft2232_spi: Revise error message ...................................................................... ft2232_spi: Revise error message Reword the message and drop the error string from libftdi. It is already printed in send_buf(). Change-Id: I125ae9ec0d5487fc26d588a7fd6c54da4ebd0d70 Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55697 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M ft2232_spi.c 1 file changed, 1 insertion(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/ft2232_spi.c b/ft2232_spi.c index 82777a2..d853b09 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -180,8 +180,7 @@ buf[1] = 0; /* Output byte ignored */ buf[2] = 0; /* Pin direction: all inputs */ if (send_buf(ftdic, buf, 3)) { - msg_perr("Unable to set pins back inputs: (%s)\n", - ftdi_get_error_string(ftdic)); + msg_perr("Unable to set pins back to inputs.\n"); ret = 1; } -- To view, visit https://review.coreboot.org/c/flashrom/+/55697 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I125ae9ec0d5487fc26d588a7fd6c54da4ebd0d70 Gerrit-Change-Number: 55697 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: 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-MessageType: merged

1 0

Change in flashrom[master]: ft2232_spi: Normalize error paths in ft2232_shutdown()
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55696 ) Change subject: ft2232_spi: Normalize error paths in ft2232_shutdown() ...................................................................... ft2232_spi: Normalize error paths in ft2232_shutdown() We missed to `free(spi_data)` on one path. It also seems odd to leak the return code of a locally used library into our common infrastruc- ture, so normalize all error paths to return 1. Change-Id: I5158d06127a9a8934b083e48b69d29c4d5a11831 Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55696 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M ft2232_spi.c 1 file changed, 7 insertions(+), 5 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/ft2232_spi.c b/ft2232_spi.c index 0d0c32c..82777a2 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -170,10 +170,10 @@ static int ft2232_shutdown(void *data) { - int f; struct ft2232_data *spi_data = (struct ft2232_data *) data; struct ftdi_context *ftdic = &spi_data->ftdic_context; unsigned char buf[3]; + int ret = 0; msg_pdbg("Releasing I/Os\n"); buf[0] = SET_BITS_LOW; @@ -182,16 +182,18 @@ if (send_buf(ftdic, buf, 3)) { msg_perr("Unable to set pins back inputs: (%s)\n", ftdi_get_error_string(ftdic)); + ret = 1; } - if ((f = ftdi_usb_close(ftdic)) < 0) { - msg_perr("Unable to close FTDI device: %d (%s)\n", f, + const int close_ret = ftdi_usb_close(ftdic); + if (close_ret < 0) { + msg_perr("Unable to close FTDI device: %d (%s)\n", close_ret, ftdi_get_error_string(ftdic)); - return f; + ret = 1; } free(spi_data); - return 0; + return ret; } static bool ft2232_spi_command_fits(const struct spi_command *cmd, size_t buffer_size) -- To view, visit https://review.coreboot.org/c/flashrom/+/55696 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5158d06127a9a8934b083e48b69d29c4d5a11831 Gerrit-Change-Number: 55696 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: 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-MessageType: merged

1 0

Change in flashrom[master]: ft2232_spi: Revise comments about output pin states
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55695 ) Change subject: ft2232_spi: Revise comments about output pin states ...................................................................... ft2232_spi: Revise comments about output pin states The meaning of the variables is easy to misunderstand as some states are merely implicit: All output pins that are not set in the `cs_bits` mask will be constantly driven low. This may be sheer coincidence as all programmers that need additional pins driven use active-low signals to enable buffers. While other pins stay low, *all* pins set in the `cs_bits` mask are supposed to be toggled during SPI transactions. Also drop some irritating dead code and try to explain things in a comment. Change-Id: I2b84ede01759c80f69d5ad17e43783d09ecd1107 Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55695 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M ft2232_spi.c 1 file changed, 21 insertions(+), 9 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/ft2232_spi.c b/ft2232_spi.c index eb98933..0d0c32c 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -86,15 +86,27 @@ #define BITMODE_BITBANG_NORMAL 1 #define BITMODE_BITBANG_SPI 2 -/* The variables cs_bits and pindir store the values for the "set data bits low byte" MPSSE command that - * sets the initial state and the direction of the I/O pins. The pin offsets are as follows: - * SCK is bit 0. - * DO is bit 1. - * DI is bit 2. - * CS is bit 3. +/* + * The variables `cs_bits` and `pindir` store the values for the + * "set data bits low byte" MPSSE command that sets the initial + * state and the direction of the I/O pins. `cs_bits` pins default + * to high and will be toggled during SPI transactions. All other + * output pins will be kept low all the time. On exit, all pins + * will be reconfigured as inputs. * - * The default values (set below in ft2232_spi_init) are used for most devices: - * value: 0x08 CS=high, DI=low, DO=low, SK=low + * The pin offsets are as follows: + * TCK/SK is bit 0. + * TDI/DO is bit 1. + * TDO/DI is bit 2. + * TMS/CS is bit 3. + * GPIOL0 is bit 4. + * GPIOL1 is bit 5. + * GPIOL2 is bit 6. + * GPIOL3 is bit 7. + * + * The default values (set below in ft2232_spi_init) are used for + * most devices: + * value: 0x08 CS=high, DI=low, DO=low, SK=low * dir: 0x0b CS=output, DI=input, DO=output, SK=output */ struct ft2232_data { @@ -219,7 +231,7 @@ msg_pspew("Assert CS#\n"); buf[i++] = SET_BITS_LOW; - buf[i++] = 0 & ~spi_data->cs_bits; /* assertive */ + buf[i++] = 0; /* assert CS# pins, all other output pins stay low */ buf[i++] = spi_data->pindir; /* WREN, OP(PROGRAM, ERASE), ADDR, DATA */ -- To view, visit https://review.coreboot.org/c/flashrom/+/55695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2b84ede01759c80f69d5ad17e43783d09ecd1107 Gerrit-Change-Number: 55695 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
by Michael Niewöhner (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Paul Menzel, Angel Pons. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 9: (1 comment) Patchset: PS4: > Forcing to run the programmer code on an unknown board would be new to flashrom. I don't see why we would suddenly need that. I haven't checked all flash drivers in flashrom but I don't remember any DMI checks or anything like that in other ec or superio drivers. Why do we need it now? -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 9 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 25 Jun 2021 12:52:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "ft2232_spi: Enhance csgpiol parameter for FT2232"
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55694 ) Change subject: Revert "ft2232_spi: Enhance csgpiol parameter for FT2232" ...................................................................... Revert "ft2232_spi: Enhance csgpiol parameter for FT2232" This reverts commit ba6575de82f091b97ea0f2efcf2f79ef3739d64f. Technically, the only thing that is wrong here is the lack of docu- mentation (manpage update). However, as this change was succeeded by a regressing fixup patch, it seems likely that the meaning of the `csgpiol` parameter was just misunderstood and these changes were not what the author intended. Change-Id: I460237b9d275b1cd1d8a069f852d17dea393b14e Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55694 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M ft2232_spi.c 1 file changed, 14 insertions(+), 26 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/ft2232_spi.c b/ft2232_spi.c index 44975db..eb98933 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -88,17 +88,10 @@ /* The variables cs_bits and pindir store the values for the "set data bits low byte" MPSSE command that * sets the initial state and the direction of the I/O pins. The pin offsets are as follows: - * TCK/SK is bit 0. - * TDI/DO is bit 1. - * TDO/DI is bit 2. - * TMS/CS is bit 3. - * GPIOL0 is bit 4. - * GPIOL1 is bit 5. - * GPIOL2 is bit 6. - * GPIOL3 is bit 7. - * - * The pin signal direction bit offsets follow the same order; 0 means that - * pin at the matching bit index is an input, 1 means pin is an output. + * SCK is bit 0. + * DO is bit 1. + * DI is bit 2. + * CS is bit 3. * * The default values (set below in ft2232_spi_init) are used for most devices: * value: 0x08 CS=high, DI=low, DO=low, SK=low @@ -459,24 +452,19 @@ } free(arg); - /* Allows setting multiple GPIOL states, for example: csgpiol=012 */ arg = extract_programmer_param("csgpiol"); if (arg) { - unsigned int ngpios = strlen(arg); - for (unsigned int i = 0; i <= ngpios; i++) { - int temp = arg[i] - '0'; - if (ngpios == 0 || (ngpios != i && (temp < 0 || temp > 3))) { - msg_perr("Error: Invalid GPIOLs specified: \"%s\".\n" - "Valid values are numbers between 0 and 3. " - "Multiple GPIOLs can be specified.\n", arg); - free(arg); - return -2; - } else { - unsigned int pin = temp + 4; - cs_bits |= 1 << pin; - pindir |= 1 << pin; - } + char *endptr; + unsigned int temp = strtoul(arg, &endptr, 10); + if (*endptr || endptr == arg || temp > 3) { + msg_perr("Error: Invalid GPIOL specified: \"%s\".\n" + "Valid values are between 0 and 3.\n", arg); + free(arg); + return -2; } + unsigned int pin = temp + 4; + cs_bits |= 1 << pin; + pindir |= 1 << pin; } free(arg); -- To view, visit https://review.coreboot.org/c/flashrom/+/55694 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I460237b9d275b1cd1d8a069f852d17dea393b14e Gerrit-Change-Number: 55694 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: Revert "ft2232_spi: Fix broken GPIOL cs_bits state (#126)"
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55693 ) Change subject: Revert "ft2232_spi: Fix broken GPIOL cs_bits state (#126)" ...................................................................... Revert "ft2232_spi: Fix broken GPIOL cs_bits state (#126)" This reverts commit a43e44b6abbe8381be3f3dd20a430973cf8b8ab5. Nothing was broken. So this breaks everything. Well, actually only the `csgpiol` parameter. But that is very obvious. `csgpiol` was added to use a GPIO pin as /CS. But this change impli- citly hardcoded /CS to ADBUS3. Change-Id: I9ecdfe227585dda74658c16c96a57dd42d1d78b4 Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55693 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M ft2232_spi.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/ft2232_spi.c b/ft2232_spi.c index 78bf0fc..44975db 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -226,7 +226,7 @@ msg_pspew("Assert CS#\n"); buf[i++] = SET_BITS_LOW; - buf[i++] = ~ 0x08 & spi_data->cs_bits; /* assert CS (3rd) bit only */ + buf[i++] = 0 & ~spi_data->cs_bits; /* assertive */ buf[i++] = spi_data->pindir; /* WREN, OP(PROGRAM, ERASE), ADDR, DATA */ -- To view, visit https://review.coreboot.org/c/flashrom/+/55693 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9ecdfe227585dda74658c16c96a57dd42d1d78b4 Gerrit-Change-Number: 55693 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2021