November 2020 - flashrom-gerrit

Change in flashrom[master]: ft2232_spi.c: Add ft2232_spi_send_multicommand()
by Simon Buhrow (Code Review) 20 Nov '20

20 Nov '20

Hello build bot (Jenkins), Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/40477 to look at the new patch set (#20). Change subject: ft2232_spi.c: Add ft2232_spi_send_multicommand() ...................................................................... ft2232_spi.c: Add ft2232_spi_send_multicommand() Pack WREN and op in one ftdi_write_data() call. Pack RD cmd and deassertion of CS# in one ftdi_write_data() call. 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 to ftdi chip 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. Furthermore the deassertion of CS# is no longer send seperately from a read cmd. This saves more than 1 minute when programming a 128MBit Flash with my config! Signed-off-by: Simon Buhrow <simon.buhrow(a)posteo.de> Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a --- M ft2232_spi.c 1 file changed, 91 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/40477/20 -- 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: 20 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> 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]: ft2232_spi.c: Add ft2232_spi_send_multicommand()
by Simon Buhrow (Code Review) 19 Nov '20

19 Nov '20

Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Add ft2232_spi_send_multicommand() ...................................................................... Patch Set 19: > So, the correct way to do this is to implement `.multicommand` > instead of `.command`. I would roughly do it as follows: > > Loop > Put write part of command into buffer. > If not last command and read part is empty, > Continue with next loop iteration. > Execute buffer. > Execute read part if any. > While not last command. I uploaded PS18 with my first draft of ft2232_spi_send_multicommand(). Having Patch 40748 in mind, I decided to pack max one read cmd into one ftdi call. This simplifies the readback pointer management as well (readarr). Furthermore I think the majority (all?) calls of ft2232_spi_send_multicommand() will have at most one read cmd (correct me if I´m wrong). (To be honest I´m not sure if ft2232_spi_send_multicommand() is ever called with read cmd in the current implementation) I´m not sure if the loop condition is safe. It´s copied from default_spi_send_multicommand(). It´s untested!! I´ll do this the next week(s). Comments are welcome. -- 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: 19 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 19 Nov 2020 16:08:00 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Add ft2232_spi_send_multicommand()
by Simon Buhrow (Code Review) 19 Nov '20

19 Nov '20

Hello build bot (Jenkins), Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/40477 to look at the new patch set (#19). Change subject: ft2232_spi.c: Add ft2232_spi_send_multicommand() ...................................................................... ft2232_spi.c: Add ft2232_spi_send_multicommand() Pack WREN and op in one ftdi_write_data() call. Pack RD cmd and deassertion of CS# in one ftdi_write_data() call. 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 to ftdi chip 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. Furthermore the deassertion of CS# is no longer send seperately from a read cmd. This saves more than 1 minute when programming a 128MBit Flash with my config! Signed-off-by: Simon Buhrow <simon.buhrow(a)posteo.de> Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a --- M ft2232_spi.c 1 file changed, 88 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/40477/19 -- 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: 19 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> 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]: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call
by Simon Buhrow (Code Review) 19 Nov '20

19 Nov '20

Hello build bot (Jenkins), Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/40477 to look at the new patch set (#18). Change subject: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call ...................................................................... ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call 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! 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, 88 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/40477/18 -- 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: 18 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> 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]: it85spi.c: Changing the order of functions
by Anastasia Klimchuk (Code Review) 18 Nov '20

18 Nov '20

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47657 ) Change subject: it85spi.c: Changing the order of functions ...................................................................... it85spi.c: Changing the order of functions This only changes the order of functions in the file, not what they are doing. With this order, prototype for it85xx_spi_send_command is not needed. Moreover the change cleans-up a way for a later work to refactor singleton states into reentrant pattern. BUG=b:172876667 TEST=builds Change-Id: Idf4241c92d90c28dd4f4ec3b7d66bda50801385a Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M it85spi.c 1 file changed, 64 insertions(+), 69 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/57/47657/1 diff --git a/it85spi.c b/it85spi.c index b998790..342a6e5 100644 --- a/it85spi.c +++ b/it85spi.c @@ -223,6 +223,70 @@ return 0; /* FIXME: Should probably return something meaningful */ } +/* According to ITE 8502 document, the procedure to follow mode is following: + * 1. write 0x00 to LPC/FWH address 0xffff_fexxh (drive CE# high) + * 2. write data to LPC/FWH address 0xffff_fdxxh (drive CE# low and MOSI + * with data) + * 3. read date from LPC/FWH address 0xffff_fdxxh (drive CE# low and get + * data from MISO) + */ +static int it85xx_spi_send_command(const struct flashctx *flash, + unsigned int writecnt, unsigned int readcnt, + const unsigned char *writearr, + unsigned char *readarr) +{ + unsigned int i; + + it85xx_enter_scratch_rom(); + /* Exit scratch ROM ONLY when programmer shuts down. Otherwise, the + * temporary flash state may halt the EC. + */ + +#ifdef LPC_IO + INDIRECT_A1(shm_io_base, (((unsigned long int)ce_high) >> 8) & 0xff); + INDIRECT_WRITE(shm_io_base, 0xFF); /* Write anything to this address.*/ + INDIRECT_A1(shm_io_base, (((unsigned long int)ce_low) >> 8) & 0xff); +#endif +#ifdef LPC_MEMORY + mmio_writeb(0, ce_high); +#endif + for (i = 0; i < writecnt; ++i) { +#ifdef LPC_IO + INDIRECT_WRITE(shm_io_base, writearr[i]); +#endif +#ifdef LPC_MEMORY + mmio_writeb(writearr[i], ce_low); +#endif + } + for (i = 0; i < readcnt; ++i) { +#ifdef LPC_IO + readarr[i] = INDIRECT_READ(shm_io_base); +#endif +#ifdef LPC_MEMORY + readarr[i] = mmio_readb(ce_low); +#endif + } +#ifdef LPC_IO + INDIRECT_A1(shm_io_base, (((unsigned long int)ce_high) >> 8) & 0xff); + INDIRECT_WRITE(shm_io_base, 0xFF); /* Write anything to this address.*/ +#endif +#ifdef LPC_MEMORY + mmio_writeb(0, ce_high); +#endif + + return 0; +} + +static const struct spi_master spi_master_it85xx = { + .max_data_read = 64, + .max_data_write = 64, + .command = it85xx_spi_send_command, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, + .write_aai = default_spi_write_aai, +}; + static int it85xx_spi_common_init(struct superio s) { chipaddr base; @@ -270,21 +334,6 @@ return 0; } -static int it85xx_spi_send_command(const struct flashctx *flash, - unsigned int writecnt, unsigned int readcnt, - const unsigned char *writearr, - unsigned char *readarr); - -static const struct spi_master spi_master_it85xx = { - .max_data_read = 64, - .max_data_write = 64, - .command = it85xx_spi_send_command, - .multicommand = default_spi_send_multicommand, - .read = default_spi_read, - .write_256 = default_spi_write_256, - .write_aai = default_spi_write_aai, -}; - int it85xx_spi_init(struct superio s) { int ret; @@ -315,58 +364,4 @@ return ret; } -/* According to ITE 8502 document, the procedure to follow mode is following: - * 1. write 0x00 to LPC/FWH address 0xffff_fexxh (drive CE# high) - * 2. write data to LPC/FWH address 0xffff_fdxxh (drive CE# low and MOSI - * with data) - * 3. read date from LPC/FWH address 0xffff_fdxxh (drive CE# low and get - * data from MISO) - */ -static int it85xx_spi_send_command(const struct flashctx *flash, - unsigned int writecnt, unsigned int readcnt, - const unsigned char *writearr, - unsigned char *readarr) -{ - unsigned int i; - - it85xx_enter_scratch_rom(); - /* Exit scratch ROM ONLY when programmer shuts down. Otherwise, the - * temporary flash state may halt the EC. - */ - -#ifdef LPC_IO - INDIRECT_A1(shm_io_base, (((unsigned long int)ce_high) >> 8) & 0xff); - INDIRECT_WRITE(shm_io_base, 0xFF); /* Write anything to this address.*/ - INDIRECT_A1(shm_io_base, (((unsigned long int)ce_low) >> 8) & 0xff); -#endif -#ifdef LPC_MEMORY - mmio_writeb(0, ce_high); -#endif - for (i = 0; i < writecnt; ++i) { -#ifdef LPC_IO - INDIRECT_WRITE(shm_io_base, writearr[i]); -#endif -#ifdef LPC_MEMORY - mmio_writeb(writearr[i], ce_low); -#endif - } - for (i = 0; i < readcnt; ++i) { -#ifdef LPC_IO - readarr[i] = INDIRECT_READ(shm_io_base); -#endif -#ifdef LPC_MEMORY - readarr[i] = mmio_readb(ce_low); -#endif - } -#ifdef LPC_IO - INDIRECT_A1(shm_io_base, (((unsigned long int)ce_high) >> 8) & 0xff); - INDIRECT_WRITE(shm_io_base, 0xFF); /* Write anything to this address.*/ -#endif -#ifdef LPC_MEMORY - mmio_writeb(0, ce_high); -#endif - - return 0; -} - #endif -- To view, visit https://review.coreboot.org/c/flashrom/+/47657 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idf4241c92d90c28dd4f4ec3b7d66bda50801385a Gerrit-Change-Number: 47657 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

2 5

Change in flashrom[master]: spi25.c: Improve spi_simple_write_cmd() diags and readability
by Edward O'Callaghan (Code Review) 17 Nov '20

17 Nov '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47452 ) Change subject: spi25.c: Improve spi_simple_write_cmd() diags and readability ...................................................................... spi25.c: Improve spi_simple_write_cmd() diags and readability Clearly initialise the cmds[] array of structures with explicit values and improve failure diagnostic line to inline failing op. BUG=b:170690915 BRANCH=none TEST=shows opcode Change-Id: Ib9fc82558035e511ee485fb5aab0bcd3e164c6ba Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M spi25.c 1 file changed, 5 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/52/47452/1 diff --git a/spi25.c b/spi25.c index 213273f..41fa3dd 100644 --- a/spi25.c +++ b/spi25.c @@ -324,20 +324,22 @@ { struct spi_command cmds[] = { { - .readarr = 0, .writecnt = JEDEC_WREN_OUTSIZE, .writearr = (const unsigned char[]){ JEDEC_WREN }, + .readcnt = 0, + .readarr = NULL, }, { - .readarr = 0, .writecnt = 1, .writearr = (const unsigned char[]){ op }, + .readcnt = 0, + .readarr = NULL, }, NULL_SPI_CMD, }; const int result = spi_send_multicommand(flash, cmds); if (result) - msg_cerr("%s failed during command execution\n", __func__); + msg_cerr("%s: opcode=0x%x failed during command execution\n", __func__, op); const int status = poll_delay ? spi_poll_wip(flash, poll_delay) : 0; -- To view, visit https://review.coreboot.org/c/flashrom/+/47452 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9fc82558035e511ee485fb5aab0bcd3e164c6ba Gerrit-Change-Number: 47452 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

2 5

Change in flashrom[master]: chipset_enable.c: Add Intel pch6 pid=0x1e4{1,2,3} support
by Edward O'Callaghan (Code Review) 14 Nov '20

14 Nov '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47090 ) Change subject: chipset_enable.c: Add Intel pch6 pid=0x1e4{1,2,3} support ...................................................................... chipset_enable.c: Add Intel pch6 pid=0x1e4{1,2,3} support BUG=none BRANCH=none TEST=none Change-Id: Ic69dc024e9af0c43d6b3a8213a5dc5d2f898c447 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M chipset_enable.c 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/90/47090/1 diff --git a/chipset_enable.c b/chipset_enable.c index a0ac6c1..c664918 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -1839,6 +1839,9 @@ {0x8086, 0x1c5c, B_FS, DEP, "Intel", "H61", enable_flash_pch6}, {0x8086, 0x1d40, B_FS, DEP, "Intel", "C60x/X79", enable_flash_pch6}, {0x8086, 0x1d41, B_FS, DEP, "Intel", "C60x/X79", enable_flash_pch6}, + {0x8086, 0x1e41, B_FS, OK, "Intel", "Desktop Full", enable_flash_pch6}, + {0x8086, 0x1e42, B_FS, OK, "Intel", "Mobile Full", enable_flash_pch6}, + {0x8086, 0x1e43, B_FS, OK, "Intel", "Mobile SFF", enable_flash_pch6}, {0x8086, 0x1e44, B_FS, DEP, "Intel", "Z77", enable_flash_pch7}, {0x8086, 0x1e46, B_FS, NT, "Intel", "Z75", enable_flash_pch7}, {0x8086, 0x1e47, B_FS, DEP, "Intel", "Q77", enable_flash_pch7}, -- To view, visit https://review.coreboot.org/c/flashrom/+/47090 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic69dc024e9af0c43d6b3a8213a5dc5d2f898c447 Gerrit-Change-Number: 47090 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

3 10

Change in flashrom[master]: chipset_enable.c: Add MCP61 pid=0x03e2 support
by Edward O'Callaghan (Code Review) 13 Nov '20

13 Nov '20

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47089 ) Change subject: chipset_enable.c: Add MCP61 pid=0x03e2 support ...................................................................... chipset_enable.c: Add MCP61 pid=0x03e2 support BUG=none BRANCH=none TEST=none Change-Id: Id1cb8b77b80c6291886238f91ffb7bf75e5750b3 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M chipset_enable.c 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/47089/1 diff --git a/chipset_enable.c b/chipset_enable.c index 4273478..a0ac6c1 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -1774,6 +1774,7 @@ {0x10de, 0x0367, B_L, OK, "NVIDIA", "MCP55", enable_flash_mcp55}, /* Pro */ {0x10de, 0x03e0, B_LS, OK, "NVIDIA", "MCP61", enable_flash_mcp6x_7x}, {0x10de, 0x03e1, B_LS, OK, "NVIDIA", "MCP61", enable_flash_mcp6x_7x}, + {0x10de, 0x03e2, B_LS, NT, "NVIDIA", "MCP61", enable_flash_mcp6x_7x}, {0x10de, 0x03e3, B_LS, NT, "NVIDIA", "MCP61", enable_flash_mcp6x_7x}, {0x10de, 0x0440, B_LS, NT, "NVIDIA", "MCP65", enable_flash_mcp6x_7x}, {0x10de, 0x0441, B_LS, NT, "NVIDIA", "MCP65", enable_flash_mcp6x_7x}, -- To view, visit https://review.coreboot.org/c/flashrom/+/47089 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id1cb8b77b80c6291886238f91ffb7bf75e5750b3 Gerrit-Change-Number: 47089 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

3 4

Change in flashrom[master]: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call
by Nico Huber (Code Review) 12 Nov '20

12 Nov '20

Nico Huber 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 ...................................................................... Patch Set 16: (1 comment) https://review.coreboot.org/c/flashrom/+/40477/16/ft2232_spi.c File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/16/ft2232_spi.c@497 PS16, Line 497: bufsize = max(writecnt + 19, 261 + 19); > What do you want to drop? Just the change from 260+9 to 261+19 or the whole max()-line? I meant dropping the max(). Just `bufsize = writecnt + 9` (or `+ 19`) would have worked to. The optimization is only that with the max(), you probably get less realloc() calls. But the overhead of realloc() (so far my impres- sion) is insignificant in a program that does USB transfers (which have a much higher overhead). I don't know but assume that this max(), and the fewer realloc() calls, save us about 1us per flashrom run, in the best case. (If this were a compute intensive program, avoiding realloc() would make sense.) And yes, starting with 4K and only increasing the buffer on demand would work well. But then again, realloc() really isn't that expensive. PS. Looked at PS11 now, that would work too. But needs a check that we never exceed the buffer ofc. -- 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: 16 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 12 Nov 2020 20:46:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Simon Buhrow Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

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

12 Nov '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 ...................................................................... Patch Set 16: (3 comments) Thanks. Much better than no feedback! I will work on this when I get time. May be anyone (Nico?) can give a comment on ft2232_spi.c#497 meanwhile. https://review.coreboot.org/c/flashrom/+/40477/16/ft2232_spi.c File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/16/ft2232_spi.c@172 PS16, Line 172: .multicommand = default_spi_send_multicommand, > You just need to implement this and will get both commands passed at once. I got it. This looks much better than my suggested patch! https://review.coreboot.org/c/flashrom/+/40477/16/ft2232_spi.c@497 PS16, Line 497: bufsize = max(writecnt + 19, 261 + 19); > This assumes `i` is at most 10. But there is really no guarantee because […] The optimization done in this change (to 261+19) was not due to USB-Transfer itself, but due to FTDI-Response time after every ftdi_write_data-call (which in this case is the some number as the number of USB-transfers). So this is a significant optimization. But with the good idea of implementing spi_send_multicommand for ftdi I have to check what this does with bufsize of (261+19). What do you want to drop? Just the change from 260+9 to 261+19 or the whole max()-line? I would drop the whole line (see PS11). IMHO the easiest and best way would be so set bufsize (to chunksize) to 4096. Is there any benefit of saving ca. 3.8kByte of space on a PC to the cost of more complex code? As Angel Pons suggested, this could be done in an extra patch. https://review.coreboot.org/c/flashrom/+/40477/16/ft2232_spi.c@562 PS16, Line 562: if (writearr[0] == JEDEC_WREN) { > What if the chip isn't JEDEC? 0x06 (WREN) could mean something very different. […] You are right, I was assuming JEDEC_WREN in any case. Thank´s for that hint! -- 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: 16 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 12 Nov 2020 13:45:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2020