Change in flashrom[master]: ft2232_spi.c: change the chunksize to 280

[Simon Buhrow (Code Review)]

Simon Buhrow has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

ft2232_spi.c: change the chunksize to 280

Signed-off-by: Simon Buhrow 
Change-Id: I5e096937834fc8fc8623c8c59ade529de081b72a
---
M ft2232_spi.c
1 file changed, 12 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/43282/1

diff --git a/ft2232_spi.c b/ft2232_spi.c
index 0799af4..0a8d631 100644
--- a/ft2232_spi.c
+++ b/ft2232_spi.c
@@ -398,7 +398,18 @@
    	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 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
+		*/
    	msg_perr("Unable to set chunk size (%s).\n", ftdi_get_error_string(ftdic));
    }

[Simon Buhrow (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/43282

to look at the new patch set (#2).

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

ft2232_spi.c: change the chunksize to 280

This is necessary for change 40477 (time saving patch).

/*
    	* 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
    	*/

Signed-off-by: Simon Buhrow 
Change-Id: I5e096937834fc8fc8623c8c59ade529de081b72a
---
M ft2232_spi.c
1 file changed, 12 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/43282/2

[Simon Buhrow (Code Review)]

Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

Patch Set 2:

The value of 280 is the minimum size needed to realize less ftdi_write_data() calls in patch 40477.

Nevertheless a value of 4096 would be reasonable as well.
In fact I can´t see any real need to set chunksize smaller than the default value (here: default equals to max) of the ftdi chunksize which equals to the ftdi buffer size (fix hardware buffer!).

A value of 4096 should result in the same behavior. However 280 is like having (/showing to have) some more control and knowledge about what happens.

I do not prefer any of the options. As there was possibly a reason to set chunksize smaller than the default value I chose 280.

[Simon Buhrow (Code Review)]

Hello build bot (Jenkins), Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/43282

to look at the new patch set (#3).

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

ft2232_spi.c: change the chunksize to 280

This is necessary for change 40477 (time saving patch).

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

Signed-off-by: Simon Buhrow 
Change-Id: I5e096937834fc8fc8623c8c59ade529de081b72a
---
M ft2232_spi.c
1 file changed, 12 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/43282/3

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

Patch Set 3: Code-Review+2

(2 comments)
> Patch Set 2:
>
> The value of 280 is the minimum size needed to realize less ftdi_write_data() calls in patch 40477.
>
> Nevertheless a value of 4096 would be reasonable as well.
> In fact I can´t see any real need to set chunksize smaller than the default value (here: default equals to max) of the ftdi chunksize which equals to the ftdi buffer size (fix hardware buffer!).
>
> A value of 4096 should result in the same behavior. However 280 is like having (/showing to have) some more control and knowledge about what happens.
>
> I do not prefer any of the options. As there was possibly a reason to set chunksize smaller than the default value I chose 280.

I'm not sure if there's any particular reason to reduce the default bufsize.

https://review.coreboot.org/c/flashrom/+/43282/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/43282/3//COMMIT_MSG@9 
PS3, Line 9: change 40477
CB:40477

https://review.coreboot.org/c/flashrom/+/43282/3//COMMIT_MSG@20 
PS3, Line 20: 
two blank lines, remove one?

[Simon Buhrow (Code Review)]

Hello build bot (Jenkins), Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/43282

to look at the new patch set (#4).

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

ft2232_spi.c: change the chunksize to 280

This is necessary for change CB:40477 (time saving patch).

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

Signed-off-by: Simon Buhrow 
Change-Id: I5e096937834fc8fc8623c8c59ade529de081b72a
---
M ft2232_spi.c
1 file changed, 12 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/43282/4

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

Patch Set 4:

Too bad these things aren't well documented. Maybe the original
FTDI library has some hint why one can set the size at all?

If we keep the chunksize setting, I would make this part of
the write command. e.g. keep a global variable with the last
set chunksize and then try to increase it everytime we need
something higher. Or the other way around, #define a maximum
and then check that this maximum isn't exceeded in the com-
mand processing.

[Simon Buhrow (Code Review)]

Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

Patch Set 4:

I checked the libftdi again and could not find any hint why this should be used.
Furhtermore I checked D2XX_Programmer's_Guide from FTDI and could not find any such function (https://www.ftdichip.com/Support/Documents/ProgramGuides/D2XX_Programmer%27s...).

So may be the easiest and cleanest way is to set it to 4096 or to not set it at all (and use the libfdti default which is again 4096)?

Is there any argument to stay with setting the chunksize lower than 4096? (again as mentioned before: I chose 280 as there might has been any reason to set it lower than 4096. But if no one has an idea why, I would opt to set it to 4096).

[Edward O'Callaghan (Code Review)]

Attention is currently required from: Simon Buhrow.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

Patch Set 4:

(1 comment)

File ft2232_spi.c:

https://review.coreboot.org/c/flashrom/+/43282/comment/339d3e23_787f01a5 
PS4, Line 401: 280)
should this be a define up at the top of the file with the comment above as the rational for the value?

[Angel Pons (Code Review)]

Attention is currently required from: Simon Buhrow, Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

Patch Set 4: Code-Review+1

(2 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/43282/comment/cfa1a450_5be3f683 
PS4, Line 9: This is necessary for change CB:40477 (time saving patch).
Does CB:40477 require this change? If so, please reorder the commits. You can reorder the last two commits using `git rebase`: https://stackoverflow.com/a/49452944

File ft2232_spi.c:

https://review.coreboot.org/c/flashrom/+/43282/comment/5c38a5fe_ba13c8cc 
PS4, Line 401: 280)
> should this be a define up at the top of the file with the comment above as the rational for the val […]

Good idea. The define could even be spelled out:

#define FTDI_CHUNK_SIZE (9 + 1 + 9 + 1 + 4 + 256)

[Simon Buhrow (Code Review)]

Attention is currently required from: Edward O'Callaghan, Angel Pons.
Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

Patch Set 4:

(1 comment)

File ft2232_spi.c:

https://review.coreboot.org/c/flashrom/+/43282/comment/cac5a1f3_ec0f387f 
PS4, Line 401: 280)
> Good idea. The define could even be spelled out: […]

Yepp, good idea!

However this could become obsolet. see patch 40477

[Simon Buhrow (Code Review)]

Attention is currently required from: Edward O'Callaghan, Angel Pons.
Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 280
......................................................................

Patch Set 4:

(1 comment)

Patchset:

PS4: 
Sorry, I was wrong: It is not getting obsolete but important because of https://review.coreboot.org/c/flashrom/+/40477

Change in flashrom[master]: ft2232_spi.c: change the chunksize to 4096

[Simon Buhrow (Code Review)]

Attention is currently required from: Edward O'Callaghan, Angel Pons.
Hello build bot (Jenkins), Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/43282

to look at the new patch set (#5).

Change subject: ft2232_spi.c: change the chunksize to 4096
......................................................................

ft2232_spi.c: change the chunksize to 4096

4096 Bytes is the hardware buffer size of the FT2232 chip.

This is necessary to get full benefit of time saving patch
https://review.coreboot.org/c/flashrom/+/40477

Signed-off-by: Simon Buhrow 
Change-Id: I5e096937834fc8fc8623c8c59ade529de081b72a
---
M ft2232_spi.c
1 file changed, 12 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/43282/5

Change in flashrom[master]: ft2232_spi.c: change the chunksize to 4096

[Simon Buhrow (Code Review)]

Attention is currently required from: Edward O'Callaghan, Angel Pons.
Hello build bot (Jenkins), Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/43282

to look at the new patch set (#6).

Change subject: ft2232_spi.c: change the chunksize to 4096
......................................................................

ft2232_spi.c: change the chunksize to 4096

4096 Bytes is the hardware buffer size of the FT2232 chip.

This is necessary to get full benefit of time saving patch
https://review.coreboot.org/c/flashrom/+/40477

Signed-off-by: Simon Buhrow 
Change-Id: I5e096937834fc8fc8623c8c59ade529de081b72a
---
M ft2232_spi.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/43282/6

Change in flashrom[master]: ft2232_spi.c: change the chunksize to 4096

[Nico Huber (Code Review)]

Attention is currently required from: Simon Buhrow, Edward O'Callaghan, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 4096
......................................................................

Patch Set 6:

(1 comment)

Patchset:

PS6: 
Turns out this is not about the FTDI hardware but how much data
libftdi hands over to libusb at once. As 4096 is the default,
I guess we can just remove it.

Change in flashrom[master]: ft2232_spi.c: change the chunksize to 4096

[Simon Buhrow (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 4096
......................................................................

Patch Set 6:

(1 comment)

Patchset:

PS6:
> Turns out this is not about the FTDI hardware but how much data […]

Yes, you are right.

Change in flashrom[master]: ft2232_spi.c: change the chunksize to 4096

[Simon Buhrow (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 4096
......................................................................

Patch Set 6:

(1 comment)

Patchset:

PS6: 
Became obsolete s. https://review.coreboot.org/c/flashrom/+/55683

Change in flashrom[master]: ft2232_spi.c: change the chunksize to 4096

[Simon Buhrow (Code Review)]

Simon Buhrow has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/43282 )

Change subject: ft2232_spi.c: change the chunksize to 4096
......................................................................

Abandoned

Became obsolete s. https://review.coreboot.org/c/flashrom/+/55683