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

Change in flashrom[master]: pcidev.c: Drop unused variable
by HAOUAS Elyes (Code Review) 24 Apr '20

24 Apr '20

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/40576 ) Change subject: pcidev.c: Drop unused variable ...................................................................... pcidev.c: Drop unused variable Change-Id: I6eea3e34ed6fc5d3fe65d5cf7e7bfc5e571bfa73 Signed-off-by: Elyes HAOUAS <ehaouas(a)noos.fr> --- M pcidev.c 1 file changed, 1 insertion(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/40576/1 diff --git a/pcidev.c b/pcidev.c index 54c1fd3..e13b78c 100644 --- a/pcidev.c +++ b/pcidev.c @@ -189,7 +189,6 @@ char *msg = NULL; int found = 0; int i; - uintptr_t addr = 0; if (pci_init_common() != 0) return NULL; @@ -229,7 +228,7 @@ /* FIXME: We should count all matching devices, not * just those with a valid BAR. */ - if ((addr = pcidev_readbar(dev, bar)) != 0) { + if (pcidev_readbar(dev, bar) != 0) { found_dev = dev; found++; } -- To view, visit https://review.coreboot.org/c/flashrom/+/40576 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6eea3e34ed6fc5d3fe65d5cf7e7bfc5e571bfa73 Gerrit-Change-Number: 40576 Gerrit-PatchSet: 1 Gerrit-Owner: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-MessageType: newchange

4 4

Change in flashrom[master]: ft2232_spi.c - Pack WREN and op in one ftdi_write_data() call to save...
by Simon Buhrow (Code Review) 21 Apr '20

21 Apr '20

Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c - Pack WREN and op in one ftdi_write_data() call to save programming time ...................................................................... Patch Set 3: (1 comment) https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c@549 PS2, Line 549: return 0; > I missed that buf is static, so this will work out fine. Well, in the usual use case with maximum buf size equal to normal page program operations, I agree. But if writecnt gets bigger than page program (plus cmd and addr bytes), realloc is called again. And then the WREN which was saved at the old buf memory location will get lost, right? (although buf is static) I think at the moment there does not happen bigger writecnt than 256Byte page program?! Nevertheless it might be better to make this somewhat cleaner. I see the following options: A) to catch bigger writecnt in line 477 OR B) to filter them in line 481 OR C) to set bufsize fix to 4096. This is the size of the FIFO buffer of the ftdi2232 [1]. So this is the biggest size which ever can be send to the ftdi chip. Sure, this would lead to some overhead in current case of use only page program size, but it might be OK and lead to a simpler code. I would prefer option C) and add it to this commit. Or are there any other preferences? [1] https://www.ftdichip.com/Support/Documents/DataSheets/ICs/DS_FT2232H.pdf -> chapter 4.2 -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 3 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 21 Apr 2020 07:56:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Simon Buhrow Comment-In-Reply-To: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c - Pack WREN and op in one ftdi_write_data() call to save...
by Patrick Georgi (Code Review) 20 Apr '20

20 Apr '20

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c - Pack WREN and op in one ftdi_write_data() call to save programming time ...................................................................... Patch Set 3: (1 comment) https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c@549 PS2, Line 549: return 0; > Oh, good comment. […] I missed that buf is static, so this will work out fine. -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 3 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 20 Apr 2020 14:37:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Simon Buhrow Comment-In-Reply-To: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c - Pack WREN and op in one ftdi_write_data() call to save...
by Simon Buhrow (Code Review) 20 Apr '20

20 Apr '20

Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c - Pack WREN and op in one ftdi_write_data() call to save programming time ...................................................................... Patch Set 3: (4 comments) > Patch Set 2: > > (4 comments) https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c@473 PS2, Line 473: > tab for consistency Done https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c@482 PS2, Line 482: /* 280 Byte = (9 Byte CMD + 1 Byte WREN) + (9 Byte CMD + 1 Byte op) + 4 Byte Addr + 256 Byte PageWrite-data > wrap differently so it fits on 80 columns? (also in line 548) Done https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c@546 PS2, Line 546: > no trailing white space (also elsewhere) Done https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c@549 PS2, Line 549: return 0; > so you're filling in buf, then leaving it hanging and somehow that data re-appears on the next run. […] Oh, good comment. I was assuming that realloc is called only once (as I assume that the highest writecnt value refers to page write). But if there will be any bigger write operations it will fail. Is there a chance for calls with more than one 256Byte page to write? If yes: May be it´s OK to save only a flag which indicates if WREN was passed in the previous call. And according to that flag the WREN cmd is prepend or not. -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 3 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 20 Apr 2020 11:37:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c - Pack WREN and op in one ftdi_write_data() call to save...
by Simon Buhrow (Code Review) 20 Apr '20

20 Apr '20

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/40477 to look at the new patch set (#3). Change subject: ft2232_spi.c - Pack WREN and op in one ftdi_write_data() call to save programming time ...................................................................... ft2232_spi.c - Pack WREN and op in one ftdi_write_data() call to save programming time Every ftdi_write_data() call is quite time consuming as the ftdi-chips seems to take always 2-3ms to respond. This leads to what the comment already says: Minimize USB transfers by packing as many commands as possible together. So I packed the WREN command together with the following operation which can be program or erase operation. This saves about 1 minute when programming a 128MBit Flash with my config! See timings WREN in extra ftdi_write_data() call: https://ibb.co/0c1J25d WREN packed with op: https://ibb.co/7G1hLkj I´m using ftdi-2232H chip. That´s why I put it at this place. If anyone has a good overview about all programmers: This could be implemented in spi_write_cmd() in case that it is compatible to all programmers or this principle could be transfered to other programmers which act in a similar way. Signed-off-by: Simon Buhrow <simon.buhrow(a)posteo.de> Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a --- M ft2232_spi.c 1 file changed, 19 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/40477/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 3 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a
by Simon Buhrow (Code Review) 20 Apr '20

20 Apr '20

Simon Buhrow has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/40521 ) Change subject: Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a ...................................................................... Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Change-Id: I218856c79844b31b302ec06f03db5599d274712e --- M ft2232_spi.c 1 file changed, 19 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/40521/1 diff --git a/ft2232_spi.c b/ft2232_spi.c index 520eb6e..9017a0f 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -398,7 +398,10 @@ msg_perr("Unable to set latency timer (%s).\n", ftdi_get_error_string(ftdic)); } - if (ftdi_write_data_set_chunksize(ftdic, 270)) { + if (ftdi_write_data_set_chunksize(ftdic, 280)) { + /* 280 Byte = (9 Byte CMD + 1 Byte WREN) + (9 Byte CMD + 1 Byte op) + 4 Byte Addr + * + 256 Byte PageWrite-data + with op: PageProgram or Erase; CMD: FTDI-Chip commands*/ msg_perr("Unable to set chunk size (%s).\n", ftdi_get_error_string(ftdic)); } @@ -467,7 +470,8 @@ struct ftdi_context *ftdic = &ftdic_context; static unsigned char *buf = NULL; /* failed is special. We use bitwise ops, but it is essentially bool. */ - int i = 0, ret = 0, failed = 0; + static int i = 0; + int ret = 0, failed = 0; int bufsize; static int oldbufsize = 0; @@ -475,7 +479,10 @@ return SPI_INVALID_LENGTH; /* buf is not used for the response from the chip. */ - bufsize = max(writecnt + 9, 260 + 9); + bufsize = max(writecnt + 9, 280); + /* 280 Byte = (9 Byte CMD + 1 Byte WREN) + (9 Byte CMD + 1 Byte op) + 4 Byte Addr + * + 256 Byte PageWrite-data + * with op: PageProgram or Erase; CMD: FTDI-Chip commands*/ /* Never shrink. realloc() calls are expensive. */ if (bufsize > oldbufsize) { buf = realloc(buf, bufsize); @@ -538,7 +545,15 @@ buf[i++] = SET_BITS_LOW; buf[i++] = cs_bits; buf[i++] = pindir; - ret = send_buf(ftdic, buf, i); + + if (writearr[0] == JEDEC_WREN) { + /* Return to get second op (Program or Erase) without resetting buf nor i*/ + return 0; + } else { + ret = send_buf(ftdic, buf, i); + i = 0; + } + failed |= ret; if (ret) msg_perr("send_buf failed at end: %i\n", ret); -- To view, visit https://review.coreboot.org/c/flashrom/+/40521 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I218856c79844b31b302ec06f03db5599d274712e Gerrit-Change-Number: 40521 Gerrit-PatchSet: 1 Gerrit-Owner: Simon Buhrow Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: ft2232_spi.c - Pack WREN and op in one ftdi_write_data() call to save...
by Patrick Georgi (Code Review) 20 Apr '20

20 Apr '20

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c - Pack WREN and op in one ftdi_write_data() call to save programming time ...................................................................... Patch Set 2: (4 comments) https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c@473 PS2, Line 473: tab for consistency https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c@482 PS2, Line 482: /* 280 Byte = (9 Byte CMD + 1 Byte WREN) + (9 Byte CMD + 1 Byte op) + 4 Byte Addr + 256 Byte PageWrite-data wrap differently so it fits on 80 columns? (also in line 548) https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c@546 PS2, Line 546: no trailing white space (also elsewhere) https://review.coreboot.org/c/flashrom/+/40477/2/ft2232_spi.c@549 PS2, Line 549: return 0; so you're filling in buf, then leaving it hanging and somehow that data re-appears on the next run. Is that based on an assumption that realloc will point to the same memory range? Seems scary! -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 2 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 20 Apr 2020 07:34:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: lspcon_i2c_spi.c: Clean up some unnecessary indirection
by Edward O'Callaghan (Code Review) 19 Apr '20

19 Apr '20

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/40470 ) Change subject: lspcon_i2c_spi.c: Clean up some unnecessary indirection ...................................................................... lspcon_i2c_spi.c: Clean up some unnecessary indirection Clean up some confusion of implicit copies by indirection. This allows the caller of register_spi_master() to have it's internal state in the expected way. Also add an error when the mpu reset fails on init. BUG=b:148746232,b:153027771,b:140394053 BRANCH=none TEST=builds Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Change-Id: I66ba4ffeb696309b8ad5b5ba58650630e8feefa9 Reviewed-on: https://review.coreboot.org/c/flashrom/+/40470 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M lspcon_i2c_spi.c 1 file changed, 7 insertions(+), 11 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c index 4c85426..1ee5f37 100644 --- a/lspcon_i2c_spi.c +++ b/lspcon_i2c_spi.c @@ -406,7 +406,7 @@ return SPI_GENERIC_ERROR; } -static const struct spi_master spi_master_i2c_lspcon = { +static struct spi_master spi_master_i2c_lspcon = { .max_data_read = 16, .max_data_write = 12, .command = lspcon_i2c_spi_send_command, @@ -468,14 +468,6 @@ return ret; } -static int register_lspcon_i2c_spi_master(void *ptr) -{ - struct spi_master mst = spi_master_i2c_lspcon; - mst.data = ptr; - - return register_spi_master(&mst); -} - int lspcon_i2c_spi_init(void) { int lspcon_i2c_spi_bus = get_bus(); @@ -488,8 +480,10 @@ return fd; ret |= lspcon_i2c_spi_reset_mpu_stop(fd); - if (ret) + if (ret) { + msg_perr("%s: call to reset_mpu_stop failed.\n", __func__); return ret; + } struct lspcon_i2c_spi_data *data = calloc(1, sizeof(struct lspcon_i2c_spi_data)); if (!data) { @@ -498,8 +492,10 @@ } data->fd = fd; + spi_master_i2c_lspcon.data = data; + ret |= register_shutdown(lspcon_i2c_spi_shutdown, data); - ret |= register_lspcon_i2c_spi_master(data); + ret |= register_spi_master(&spi_master_i2c_lspcon); return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/40470 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I66ba4ffeb696309b8ad5b5ba58650630e8feefa9 Gerrit-Change-Number: 40470 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Shiyu Sun <sshiyu(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: lspcon_i2c_spi.c: Clean up some unnecessary indirection
by Angel Pons (Code Review) 19 Apr '20

19 Apr '20

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40470 ) Change subject: lspcon_i2c_spi.c: Clean up some unnecessary indirection ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/40470 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I66ba4ffeb696309b8ad5b5ba58650630e8feefa9 Gerrit-Change-Number: 40470 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Shiyu Sun <sshiyu(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Sun, 19 Apr 2020 09:07:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: lspcon_i2c_spi.c: Clean up some unnecessary indirection
by Edward O'Callaghan (Code Review) 19 Apr '20

19 Apr '20

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40470 ) Change subject: lspcon_i2c_spi.c: Clean up some unnecessary indirection ...................................................................... Patch Set 2: > Patch Set 1: Code-Review+1 > > Needs a rebase To re-trigger the buildbot? Done. -- To view, visit https://review.coreboot.org/c/flashrom/+/40470 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I66ba4ffeb696309b8ad5b5ba58650630e8feefa9 Gerrit-Change-Number: 40470 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Shiyu Sun <sshiyu(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 19 Apr 2020 05:25:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2020