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

Change in flashrom[master]: Revert "ft2232_spi: Enhance csgpiol parameter for FT2232"
by Angel Pons (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Alan Green, Nico Huber, Edward O'Callaghan, Samir Ibradžić. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55694 ) Change subject: Revert "ft2232_spi: Enhance csgpiol parameter for FT2232" ...................................................................... Patch Set 1: Code-Review+2 -- 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: 1 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 14:20:41 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

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

20 Jun '21

Attention is currently required from: Alan Green, Nico Huber, Edward O'Callaghan, Samir Ibradžić. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55693 ) Change subject: Revert "ft2232_spi: Fix broken GPIOL cs_bits state (#126)" ...................................................................... Patch Set 1: Code-Review+2 -- 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: 1 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 14:20:34 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "ft2232_spi.c: Generalize GPIOL pin control"
by Angel Pons (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Alan Green, Nico Huber, Edward O'Callaghan, Samir Ibradžić. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55692 ) Change subject: Revert "ft2232_spi.c: Generalize GPIOL pin control" ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55692 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4a95317b1cf1fc6df9471d0cfb8a6a8f40964fe3 Gerrit-Change-Number: 55692 Gerrit-PatchSet: 1 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 14:20:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "ft2232_spi.c: Rename cs_bits to pinlvl"
by Angel Pons (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Alan Green, Nico Huber, Edward O'Callaghan, Samir Ibradžić. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55691 ) Change subject: Revert "ft2232_spi.c: Rename cs_bits to pinlvl" ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55691 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa5b2259ccf49ddf729d01176bacd94a95c39925 Gerrit-Change-Number: 55691 Gerrit-PatchSet: 1 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 14:20:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

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

20 Jun '21

Attention is currently required from: Alan Green, Samir Ibradžić. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55695 ) Change subject: ft2232_spi: Revise comments about output pin states ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Alan, Samir, I'm sorry about the reverts. I think it's best to restart from a sound code base. If you think it's easier to fix everything on top of the current TOT, please go ahead and ignore the reverts. It seems you were both working on the same feature, general GPIO control indipendent from CS#. But that's not what the original `csgpiol` code intended (hence the `cs` in its name). -- 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: 1 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 14:09:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

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

20 Jun '21

Attention is currently required from: Alan Green, Edward O'Callaghan, Angel Pons, Samir Ibradžić. Hello Alan Green, Edward O'Callaghan, Angel Pons, Samir Ibradžić, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/55695 to review the following change. 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> --- M ft2232_spi.c 1 file changed, 21 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/95/55695/1 diff --git a/ft2232_spi.c b/ft2232_spi.c index dba979b..e546f6c 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 { @@ -218,7 +230,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: 1 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-MessageType: newchange

1 0

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

20 Jun '21

Attention is currently required from: Alan Green, Edward O'Callaghan, Angel Pons, Samir Ibradžić. Hello Alan Green, Edward O'Callaghan, Angel Pons, Samir Ibradžić, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/55694 to review the following change. 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> --- M ft2232_spi.c 1 file changed, 14 insertions(+), 26 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/94/55694/1 diff --git a/ft2232_spi.c b/ft2232_spi.c index 4a3a5a4..dba979b 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: 1 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-MessageType: newchange

1 0

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

20 Jun '21

Attention is currently required from: Alan Green, Edward O'Callaghan, Angel Pons, Samir Ibradžić. Hello Alan Green, Edward O'Callaghan, Angel Pons, Samir Ibradžić, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/55693 to review the following change. 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> --- M ft2232_spi.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/55693/1 diff --git a/ft2232_spi.c b/ft2232_spi.c index 2eb735a..4a3a5a4 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -225,7 +225,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: 1 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: Revert "ft2232_spi.c: Generalize GPIOL pin control"
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Alan Green, Edward O'Callaghan, Angel Pons, Samir Ibradžić. Hello Alan Green, Edward O'Callaghan, Angel Pons, Samir Ibradžić, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/55692 to review the following change. Change subject: Revert "ft2232_spi.c: Generalize GPIOL pin control" ...................................................................... Revert "ft2232_spi.c: Generalize GPIOL pin control" This reverts commit 3207844ec0b5dc16f9ae9ee45294213dbf6d060b. It used the `cs_bits` variable for its own purpose (not CS) which was only possible because the `cs_bits` semantics were broken earlier. It also lacks an update to the manpage. Change-Id: I4a95317b1cf1fc6df9471d0cfb8a6a8f40964fe3 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M ft2232_spi.c 1 file changed, 1 insertion(+), 43 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/92/55692/1 diff --git a/ft2232_spi.c b/ft2232_spi.c index d8f1bc3..2eb735a 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -459,7 +459,7 @@ } free(arg); - /* Allows setting multiple GPIOL pins to high, for example: csgpiol=012 */ + /* Allows setting multiple GPIOL states, for example: csgpiol=012 */ arg = extract_programmer_param("csgpiol"); if (arg) { unsigned int ngpios = strlen(arg); @@ -480,48 +480,6 @@ } free(arg); - /* Allows setting GPIOL pins high, low or input (high-z) */ - arg = extract_programmer_param("gpiol"); - if (arg) { - int ok = 0; - if (strlen(arg) == 4) { - ok = 1; - for (int i = 0; i < 4; i++) { - unsigned int pin = i + 4; - switch (toupper(arg[i])) { - case 'H': - cs_bits |= 1 << pin; - pindir |= 1 << pin; - break; - case 'L': - cs_bits &= ~(1 << pin); - pindir |= 1 << pin; - break; - case 'Z': - pindir &= ~(1 << pin); - break; - case 'X': - break; - default: - ok = 0; - } - } - } - if (!ok) { - msg_perr("Error: Invalid GPIOLs specified: \"%s\".\n" - "Valid values are 4 character strings of H, L, Z and X.\n" - " H - Set GPIOL output high\n" - " L - Set GPIOL output low\n" - " Z - Set GPIOL as input (high impedance)\n" - " X - Leave as programmer default\n" - "Example: gpiol=LZXH drives GPIOL 0 low, and GPIOL 3 high, sets GPIOL 1\n" - "to an input and leaves GPIOL 2 set according to the programmer type.\n", arg); - free(arg); - return -2; - } - } - free(arg); - msg_pdbg("Using device type %s %s ", get_ft2232_vendorname(ft2232_vid, ft2232_type), get_ft2232_devicename(ft2232_vid, ft2232_type)); -- To view, visit https://review.coreboot.org/c/flashrom/+/55692 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4a95317b1cf1fc6df9471d0cfb8a6a8f40964fe3 Gerrit-Change-Number: 55692 Gerrit-PatchSet: 1 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: Revert "ft2232_spi.c: Rename cs_bits to pinlvl"
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Alan Green, Edward O'Callaghan, Angel Pons, Samir Ibradžić. Hello Alan Green, Edward O'Callaghan, Angel Pons, Samir Ibradžić, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/55691 to review the following change. Change subject: Revert "ft2232_spi.c: Rename cs_bits to pinlvl" ...................................................................... Revert "ft2232_spi.c: Rename cs_bits to pinlvl" This reverts commit 180079632b15758ca0bbbe5782611bd4573e11d8. The renaming only seemed to match the code because of earlier introduced regressions (see following reverts). For proper support of the `gpiol` feature, we'd likely need both a `cs_bits` and a `pinlvl` variable. Change-Id: Ifa5b2259ccf49ddf729d01176bacd94a95c39925 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M ft2232_spi.c 1 file changed, 14 insertions(+), 14 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/55691/1 diff --git a/ft2232_spi.c b/ft2232_spi.c index 696a234..d8f1bc3 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -86,7 +86,7 @@ #define BITMODE_BITBANG_NORMAL 1 #define BITMODE_BITBANG_SPI 2 -/* The variables pinlvl and pindir store the values for the "set data bits low byte" MPSSE command that +/* 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. @@ -105,7 +105,7 @@ * dir: 0x0b CS=output, DI=input, DO=output, SK=output */ struct ft2232_data { - uint8_t pinlvl; + uint8_t cs_bits; uint8_t pindir; struct ftdi_context ftdic_context; }; @@ -225,7 +225,7 @@ msg_pspew("Assert CS#\n"); buf[i++] = SET_BITS_LOW; - buf[i++] = ~ 0x08 & spi_data->pinlvl; /* assert CS (3rd) bit only */ + buf[i++] = ~ 0x08 & spi_data->cs_bits; /* assert CS (3rd) bit only */ buf[i++] = spi_data->pindir; /* WREN, OP(PROGRAM, ERASE), ADDR, DATA */ @@ -247,7 +247,7 @@ /* Add final de-assert CS# */ msg_pspew("De-assert CS#\n"); buf[i++] = SET_BITS_LOW; - buf[i++] = spi_data->pinlvl; + buf[i++] = spi_data->cs_bits; buf[i++] = spi_data->pindir; /* continue if there is no read-cmd and further cmds exist */ @@ -316,7 +316,7 @@ char *arg; double mpsse_clk; - uint8_t pinlvl = 0x08; + uint8_t cs_bits = 0x08; uint8_t pindir = 0x0b; struct ftdi_context ftdic; struct ft2232_data *spi_data; @@ -338,7 +338,7 @@ /* JTAGkey(2) needs to enable its output via Bit4 / GPIOL0 * value: 0x18 OE=high, CS=high, DI=low, DO=low, SK=low * dir: 0x1b OE=output, CS=output, DI=input, DO=output, SK=output */ - pinlvl = 0x18; + cs_bits = 0x18; pindir = 0x1b; } else if (!strcasecmp(arg, "picotap")) { ft2232_vid = GOEPEL_VID; @@ -356,7 +356,7 @@ /* In its default configuration it is a jtagkey clone */ ft2232_type = FTDI_FT2232H_PID; channel_count = 2; - pinlvl = 0x18; + cs_bits = 0x18; pindir = 0x1b; } else if (!strcasecmp(arg, "openmoko")) { ft2232_vid = FIC_VID; @@ -369,7 +369,7 @@ /* arm-usb-ocd(-h) has an output buffer that needs to be enabled by pulling ADBUS4 low. * value: 0x08 #OE=low, CS=high, DI=low, DO=low, SK=low * dir: 0x1b #OE=output, CS=output, DI=input, DO=output, SK=output */ - pinlvl = 0x08; + cs_bits = 0x08; pindir = 0x1b; } else if (!strcasecmp(arg, "arm-usb-tiny")) { ft2232_vid = OLIMEX_VID; @@ -380,7 +380,7 @@ ft2232_type = OLIMEX_ARM_OCD_H_PID; channel_count = 2; /* See arm-usb-ocd */ - pinlvl = 0x08; + cs_bits = 0x08; pindir = 0x1b; } else if (!strcasecmp(arg, "arm-usb-tiny-h")) { ft2232_vid = OLIMEX_VID; @@ -473,7 +473,7 @@ return -2; } else { unsigned int pin = temp + 4; - pinlvl |= 1 << pin; + cs_bits |= 1 << pin; pindir |= 1 << pin; } } @@ -490,11 +490,11 @@ unsigned int pin = i + 4; switch (toupper(arg[i])) { case 'H': - pinlvl |= 1 << pin; + cs_bits |= 1 << pin; pindir |= 1 << pin; break; case 'L': - pinlvl &= ~(1 << pin); + cs_bits &= ~(1 << pin); pindir |= 1 << pin; break; case 'Z': @@ -600,7 +600,7 @@ msg_pdbg("Set data bits\n"); buf[0] = SET_BITS_LOW; - buf[1] = pinlvl; + buf[1] = cs_bits; buf[2] = pindir; if (send_buf(&ftdic, buf, 3)) { ret = -8; @@ -612,7 +612,7 @@ msg_perr("Unable to allocate space for SPI master data\n"); return SPI_GENERIC_ERROR; } - spi_data->pinlvl = pinlvl; + spi_data->cs_bits = cs_bits; spi_data->pindir = pindir; spi_data->ftdic_context = ftdic; -- To view, visit https://review.coreboot.org/c/flashrom/+/55691 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa5b2259ccf49ddf729d01176bacd94a95c39925 Gerrit-Change-Number: 55691 Gerrit-PatchSet: 1 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2021