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

Change in flashrom[master]: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call
by Angel Pons (Code Review) July 7, 2020

July 7, 2020

Angel Pons 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 13: (1 comment) https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c@403 PS11, Line 403: if (ftdi_write_data_set_chunksize(ftdic, 280)) { : /* : * 280 bytes = : * + 9 B (CMD) : * + 1 B (WREN) : * + 9 B (CMD) : * + 1 B (op) : * + 4 B (addr) : * + 256 B (page data) : * : * With op: PageProgram or Erase; CMD: FTDI-Chip commands : */ > This goes with the time saving changes in ft2232_spi_send_command() below. […] I don't recall suggesting that (my memory is terrible). In any case, the 1st commit would be much easier to +2 and submit than the 3rd commit (I still need to understand and test it) -- 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: 13 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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, 07 Jul 2020 13:30:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Simon Buhrow Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> 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) July 7, 2020

July 7, 2020

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 13: (4 comments) Thanks for your ideas! https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c@403 PS11, Line 403: if (ftdi_write_data_set_chunksize(ftdic, 280)) { : /* : * 280 bytes = : * + 9 B (CMD) : * + 1 B (WREN) : * + 9 B (CMD) : * + 1 B (op) : * + 4 B (addr) : * + 256 B (page data) : * : * With op: PageProgram or Erase; CMD: FTDI-Chip commands : */ > Increasing the chunksize can be the 1st commit This goes with the time saving changes in ft2232_spi_send_command() below. That´s why I think your proposed 1st and 3rd commit go together. https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c@481 PS11, Line 481: static unsigned char buf[FTDI_HW_BUFFER_SIZE]; > Using a fixed-size buffer can be the 2nd commit Done. OK, I see that this does not necessarily go with the time saving changes! Thanks! https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c@483 PS11, Line 483: static int i = 0; > This line is part of the 3rd commit Done https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c@541 PS11, Line 541: 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; : } > This is the rest of the 3rd commit Done. This is the main part of time saving change. -- 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: 13 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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, 07 Jul 2020 13:11:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> 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) July 7, 2020

July 7, 2020

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 (#13). 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, 36 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/40477/13 -- 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: 13 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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]: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call
by Simon Buhrow (Code Review) July 7, 2020

July 7, 2020

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 (#12). 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! 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, 36 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/77/40477/12 -- 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: 12 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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]: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call
by Angel Pons (Code Review) July 4, 2020

July 4, 2020

Angel Pons 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 11: (5 comments) > Patch Set 11: > > Anyone having time to give some feedback? Well, I see three different changes on this commit, and would prefer to split them up. https://review.coreboot.org/c/flashrom/+/40477/8//COMMIT_MSG Commit Message: https://review.coreboot.org/c/flashrom/+/40477/8//COMMIT_MSG@17 PS8, Line 17: https://ibb.co/0c1J25d > How do I better? Is there a better way to provide images? Just let me know. I wouldn't provide them in a commit message https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c@403 PS11, Line 403: if (ftdi_write_data_set_chunksize(ftdic, 280)) { : /* : * 280 bytes = : * + 9 B (CMD) : * + 1 B (WREN) : * + 9 B (CMD) : * + 1 B (op) : * + 4 B (addr) : * + 256 B (page data) : * : * With op: PageProgram or Erase; CMD: FTDI-Chip commands : */ Increasing the chunksize can be the 1st commit https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c@481 PS11, Line 481: static unsigned char buf[FTDI_HW_BUFFER_SIZE]; Using a fixed-size buffer can be the 2nd commit https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c@483 PS11, Line 483: static int i = 0; This line is part of the 3rd commit https://review.coreboot.org/c/flashrom/+/40477/11/ft2232_spi.c@541 PS11, Line 541: 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; : } This is the rest of the 3rd commit -- 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: 11 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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: Sat, 04 Jul 2020 18:30:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Simon Buhrow Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0