Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
ft2232_spi.c: align with Chrome OS flashrom
Brings over various changes: - Limit servo channel selection to valid range - Use DIS_DIV_5 constant - Update some comments - Wrap long lines
Signed-off-by: Nikolai Artemiev nartemiev@google.com Change-Id: I24c20e9b5d7e661d0180699bbd0d1447f6bf816f --- M ft2232_spi.c 1 file changed, 20 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/42796/1
diff --git a/ft2232_spi.c b/ft2232_spi.c index 87e6057..083e58f 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -110,7 +110,8 @@ { int i; for (i = 0; devs_ft2232spi[i].vendor_name != NULL; i++) { - if ((devs_ft2232spi[i].device_id == ft2232_type) && (devs_ft2232spi[i].vendor_id == ft2232_vid)) + if ((devs_ft2232spi[i].device_id == ft2232_type) + && (devs_ft2232spi[i].vendor_id == ft2232_vid)) return devs_ft2232spi[i].device_name; } return "unknown device"; @@ -120,7 +121,8 @@ { int i; for (i = 0; devs_ft2232spi[i].vendor_name != NULL; i++) { - if ((devs_ft2232spi[i].device_id == ft2232_type) && (devs_ft2232spi[i].vendor_id == ft2232_vid)) + if ((devs_ft2232spi[i].device_id == ft2232_type) + && (devs_ft2232spi[i].vendor_id == ft2232_vid)) return devs_ft2232spi[i].vendor_name; } return "unknown vendor"; @@ -132,7 +134,8 @@ int r; r = ftdi_write_data(ftdic, (unsigned char *) buf, size); if (r < 0) { - msg_perr("ftdi_write_data: %d, %s\n", r, ftdi_get_error_string(ftdic)); + msg_perr("ftdi_write_data: %d, %s\n", r, + ftdi_get_error_string(ftdic)); return 1; } return 0; @@ -146,7 +149,8 @@ while (size > 0) { r = ftdi_read_data(ftdic, (unsigned char *) buf, size); if (r < 0) { - msg_perr("ftdi_read_data: %d, %s\n", r, ftdi_get_error_string(ftdic)); + msg_perr("ftdi_read_data: %d, %s\n", r, + ftdi_get_error_string(ftdic)); return 1; } buf += r; @@ -183,8 +187,8 @@ enum ftdi_interface ft2232_interface = INTERFACE_A; /* * The 'H' chips can run with an internal clock of either 12 MHz or 60 MHz, - * but the non-H chips can only run at 12 MHz. We enable the divide-by-5 - * prescaler on the former to run on the same speed. + * but the non-H chips can only run at 12 MHz. We disable the divide-by-5 + * prescaler on 'H' chips so they run at 60MHz. */ uint8_t clock_5x = 1; /* In addition to the prescaler mentioned above there is also another @@ -193,7 +197,8 @@ * div = (1 + x) * 2 <-> x = div / 2 - 1 * Hence the expressible divisors are all even numbers between 2 and * 2^17 (=131072) resulting in SCK frequencies of 6 MHz down to about - * 92 Hz for 12 MHz inputs. + * 92 Hz for 12 MHz inputs and 30 MHz down to about 458 Hz for 60 MHz + * inputs. */ uint32_t divisor = DEFAULT_DIVISOR; int f; @@ -268,14 +273,17 @@ } else if (!strcasecmp(arg, "google-servo")) { ft2232_vid = GOOGLE_VID; ft2232_type = GOOGLE_SERVO_PID; + channel_count = 2; } else if (!strcasecmp(arg, "google-servo-v2")) { ft2232_vid = GOOGLE_VID; ft2232_type = GOOGLE_SERVO_V2_PID1; + channel_count = 2; /* Default divisor is too fast, and chip ID fails */ divisor = 6; } else if (!strcasecmp(arg, "google-servo-v2-legacy")) { ft2232_vid = GOOGLE_VID; ft2232_type = GOOGLE_SERVO_V2_PID0; + channel_count = 2; } else if (!strcasecmp(arg, "flyswatter")) { ft2232_type = FTDI_FT2232H_PID; channel_count = 2; @@ -381,7 +389,8 @@ free(arg);
if (f < 0 && f != -5) { - msg_perr("Unable to open FTDI device: %d (%s).\n", f, ftdi_get_error_string(ftdic)); + msg_perr("Unable to open FTDI device: %d (%s)\n", f, + ftdi_get_error_string(ftdic)); return -4; }
@@ -408,7 +417,7 @@
if (clock_5x) { msg_pdbg("Disable divide-by-5 front stage\n"); - buf[0] = 0x8a; /* Disable divide-by-5. DIS_DIV_5 in newer libftdi */ + buf[0] = DIS_DIV_5; if (send_buf(ftdic, buf, 1)) { ret = -5; goto ftdi_err; @@ -453,7 +462,8 @@
ftdi_err: if ((f = ftdi_usb_close(ftdic)) < 0) { - msg_perr("Unable to close FTDI device: %d (%s)\n", f, ftdi_get_error_string(ftdic)); + msg_perr("Unable to close FTDI device: %d (%s)\n", f, + ftdi_get_error_string(ftdic)); } return ret; }
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c@420 PS1, Line 420: DIS_DIV_5; was this not using the define before because of needing to support older libftdi on other platforms perhaps?
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c@420 PS1, Line 420: DIS_DIV_5;
was this not using the define before because of needing to support older libftdi on other platforms […]
Yeah, it was switched to use DIS_DIV_5 and then switched back in `05151b6dcbba16746aa803069dd6046a82e33599` to support older libftdi. However that was ~6 years ago, and the constant was added to libftdi ~9 years ago.
I think it's ok, but we could conditionally define DIS_DIV_5, like HAVE_FT232H on lines 29-33.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c@420 PS1, Line 420: DIS_DIV_5;
Yeah, it was switched to use DIS_DIV_5 and then switched back in `05151b6dcbba16746aa803069dd6046a82 […]
Maybe bump the minimum version requirement in meson+Makefile?
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c@420 PS1, Line 420: DIS_DIV_5;
Maybe bump the minimum version requirement in meson+Makefile?
It looks like they already depend on libftdi1, which has DIS_DIV_5 AFAICT.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
Patch Set 1: Code-Review+2
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c@420 PS1, Line 420: DIS_DIV_5;
It looks like they already depend on libftdi1, which has DIS_DIV_5 AFAICT.
OK, SGTM. Please give some time for the community to review as well.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c@276 PS1, Line 276: channel_count = 2; Which is the rationale for this change?
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c@420 PS1, Line 420: DIS_DIV_5;
OK, SGTM. Please give some time for the community to review as well.
I think we ported the few programmers using old libftdi to use the newer version, so we should be safe
Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/42796
to look at the new patch set (#2).
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
ft2232_spi.c: align with Chrome OS flashrom
Brings over various changes: - Use DIS_DIV_5 constant - Update some comments - Wrap long lines
Signed-off-by: Nikolai Artemiev nartemiev@google.com Change-Id: I24c20e9b5d7e661d0180699bbd0d1447f6bf816f --- M ft2232_spi.c 1 file changed, 17 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/42796/2
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c@276 PS1, Line 276: channel_count = 2;
Which is the rationale for this change?
Historically cros flashrom only supported channels A/B as it didn't have commit fbc71ac494a798. I added the 'channel_count = 2' restrictions when I brought over that commit since it didn't look like anyone used channels C/D, but we probably should support them.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c@276 PS1, Line 276: channel_count = 2;
Historically cros flashrom only supported channels A/B as it didn't have commit fbc71ac494a798. […]
Then why isn't it mentioned in the commit message?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/42796/1/ft2232_spi.c@276 PS1, Line 276: channel_count = 2;
Then why isn't it mentioned in the commit message?
The changes were dropped from the latest patchset
Angel Pons has submitted this change. ( https://review.coreboot.org/c/flashrom/+/42796 )
Change subject: ft2232_spi.c: align with Chrome OS flashrom ......................................................................
ft2232_spi.c: align with Chrome OS flashrom
Brings over various changes: - Use DIS_DIV_5 constant - Update some comments - Wrap long lines
Signed-off-by: Nikolai Artemiev nartemiev@google.com Change-Id: I24c20e9b5d7e661d0180699bbd0d1447f6bf816f Reviewed-on: https://review.coreboot.org/c/flashrom/+/42796 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M ft2232_spi.c 1 file changed, 17 insertions(+), 10 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/ft2232_spi.c b/ft2232_spi.c index 87e6057..65ff449 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -110,7 +110,8 @@ { int i; for (i = 0; devs_ft2232spi[i].vendor_name != NULL; i++) { - if ((devs_ft2232spi[i].device_id == ft2232_type) && (devs_ft2232spi[i].vendor_id == ft2232_vid)) + if ((devs_ft2232spi[i].device_id == ft2232_type) + && (devs_ft2232spi[i].vendor_id == ft2232_vid)) return devs_ft2232spi[i].device_name; } return "unknown device"; @@ -120,7 +121,8 @@ { int i; for (i = 0; devs_ft2232spi[i].vendor_name != NULL; i++) { - if ((devs_ft2232spi[i].device_id == ft2232_type) && (devs_ft2232spi[i].vendor_id == ft2232_vid)) + if ((devs_ft2232spi[i].device_id == ft2232_type) + && (devs_ft2232spi[i].vendor_id == ft2232_vid)) return devs_ft2232spi[i].vendor_name; } return "unknown vendor"; @@ -132,7 +134,8 @@ int r; r = ftdi_write_data(ftdic, (unsigned char *) buf, size); if (r < 0) { - msg_perr("ftdi_write_data: %d, %s\n", r, ftdi_get_error_string(ftdic)); + msg_perr("ftdi_write_data: %d, %s\n", r, + ftdi_get_error_string(ftdic)); return 1; } return 0; @@ -146,7 +149,8 @@ while (size > 0) { r = ftdi_read_data(ftdic, (unsigned char *) buf, size); if (r < 0) { - msg_perr("ftdi_read_data: %d, %s\n", r, ftdi_get_error_string(ftdic)); + msg_perr("ftdi_read_data: %d, %s\n", r, + ftdi_get_error_string(ftdic)); return 1; } buf += r; @@ -183,8 +187,8 @@ enum ftdi_interface ft2232_interface = INTERFACE_A; /* * The 'H' chips can run with an internal clock of either 12 MHz or 60 MHz, - * but the non-H chips can only run at 12 MHz. We enable the divide-by-5 - * prescaler on the former to run on the same speed. + * but the non-H chips can only run at 12 MHz. We disable the divide-by-5 + * prescaler on 'H' chips so they run at 60MHz. */ uint8_t clock_5x = 1; /* In addition to the prescaler mentioned above there is also another @@ -193,7 +197,8 @@ * div = (1 + x) * 2 <-> x = div / 2 - 1 * Hence the expressible divisors are all even numbers between 2 and * 2^17 (=131072) resulting in SCK frequencies of 6 MHz down to about - * 92 Hz for 12 MHz inputs. + * 92 Hz for 12 MHz inputs and 30 MHz down to about 458 Hz for 60 MHz + * inputs. */ uint32_t divisor = DEFAULT_DIVISOR; int f; @@ -381,7 +386,8 @@ free(arg);
if (f < 0 && f != -5) { - msg_perr("Unable to open FTDI device: %d (%s).\n", f, ftdi_get_error_string(ftdic)); + msg_perr("Unable to open FTDI device: %d (%s)\n", f, + ftdi_get_error_string(ftdic)); return -4; }
@@ -408,7 +414,7 @@
if (clock_5x) { msg_pdbg("Disable divide-by-5 front stage\n"); - buf[0] = 0x8a; /* Disable divide-by-5. DIS_DIV_5 in newer libftdi */ + buf[0] = DIS_DIV_5; if (send_buf(ftdic, buf, 1)) { ret = -5; goto ftdi_err; @@ -453,7 +459,8 @@
ftdi_err: if ((f = ftdi_usb_close(ftdic)) < 0) { - msg_perr("Unable to close FTDI device: %d (%s)\n", f, ftdi_get_error_string(ftdic)); + msg_perr("Unable to close FTDI device: %d (%s)\n", f, + ftdi_get_error_string(ftdic)); } return ret; }
-
Angel Pons (Code Review) -
Edward O'Callaghan (Code Review)
-
Nikolai Artemiev (Code Review)