August 2019 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: Add support for National Instruments USB-845x devices
by Miklós Márton (Code Review) 04 Aug '19

04 Aug '19

Miklós Márton has uploaded a new patch set (#23) to the change originally created by David Hendricks. ( https://review.coreboot.org/c/flashrom/+/25683 ) Change subject: Add support for National Instruments USB-845x devices ...................................................................... Add support for National Instruments USB-845x devices Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Signed-off-by: Miklós Márton <martonmiklosqdev(a)gmail.com> --- M Makefile M flashrom.8.tmpl M flashrom.c A ni845x_spi.c M programmer.h 5 files changed, 655 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/25683/23 -- To view, visit https://review.coreboot.org/c/flashrom/+/25683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 23 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com> Gerrit-CC: Stefan T <stefan.tauner(a)gmx.at> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: Add support for National Instruments USB-845x devices
by Miklós Márton (Code Review) 03 Aug '19

03 Aug '19

Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/25683 ) Change subject: Add support for National Instruments USB-845x devices ...................................................................... Patch Set 21: (10 comments) https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c File ni845x_spi.c: https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@150 PS21, Line 150: "USB%u::0x%04X::0x%04X::%08lX::RAW", > Nit, odd indentation (it seems you replaced 8 spaces with 2 tabs? should be 1). Done https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@151 PS21, Line 151: &usb_bus, &vid, &pid, &serial_as_number) != 4) { > here too Done https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@154 PS21, Line 154: resource_name); > and here Done https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@222 PS21, Line 222: // usb8452_io_voltages_in_100mV is a decreasing list of supported voltages > comment seems already outdated :-( Done https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@227 PS21, Line 227: i + 1 > This condition is far too complex, and, if I read this correctly, […] Done https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@222 PS21, Line 222: // usb8452_io_voltages_in_100mV is a decreasing list of supported voltages : for (i = 0; i < ARRAY_SIZE(usb8452_io_voltages_in_100mV); i++) { : if ((i == (ARRAY_SIZE(usb8452_io_voltages_in_100mV) - 1) && : usb8452_io_voltages_in_100mV[i] == IO_voltage_100mV) || : (usb8452_io_voltages_in_100mV[i] <= IO_voltage_100mV && : IO_voltage_100mV < usb8452_io_voltages_in_100mV[i + 1])) { : selected_voltage_100mV = usb8452_io_voltages_in_100mV[i]; : if (IO_voltage_100mV != usb8452_io_voltages_in_100mV[i]) { : msg_pwarn("USB-8452 IO Voltage forced to: %.1f V\n", : (float)selected_voltage_100mV / 10.0f); : } else { : msg_pinfo("USB-8452 IO Voltage set to: %.1f V\n", : (float)selected_voltage_100mV / 10.0f); : } : break; : } : } : : if (selected_voltage_100mV == 0) { : msg_pwarn("The USB-8452 does not supports the %.1f V IO voltage\n", : IOVoltage_mV / 10.0f); : selected_voltage_100mV = kNi845x12Volts; : msg_pwarn("The output voltage is set to 1.2V (this is the lowest voltage)\n"); : msg_pwarn("Supported IO voltages: "); : for (i = 0; i < ARRAY_SIZE(usb8452_io_voltages_in_100mV); i--) { : msg_pwarn("%.1fV", (float)usb8452_io_voltages_in_100mV[i] / 10.0f); : if (i != ARRAY_SIZE(usb8452_io_voltages_in_100mV) - 1) : msg_pwarn(", "); : } : msg_pwarn("\n"); : return -1; : } > I think this whole block could benefit from a better separation of […] Done https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@270 PS21, Line 270: int32 i = ni845xSpiConfigurationSetClockRate(configuration_handle, SCK_freq_in_KHz); > broken indentation Done https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@285 PS21, Line 285: if (clock_freq_read_KHz != SCK_freq_in_KHz) { : msg_pinfo("SPI clock frequency forced to: %d KHz (requested: %d KHz)\n", : (int)clock_freq_read_KHz, (int)SCK_freq_in_KHz); : } else { : msg_pinfo("SPI clock frequency set to: %d KHz\n", (int)SCK_freq_in_KHz); : } : return 0; > indentation Done https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@365 PS21, Line 365: if (CS_number < 0 || 7 < CS_number) { > Missing check of length of `CS_str`, e.g. […] Done https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@386 PS21, Line 386: free(speed_str); > move above the `if`? Done -- To view, visit https://review.coreboot.org/c/flashrom/+/25683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 21 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com> Gerrit-CC: Stefan T <stefan.tauner(a)gmx.at> Gerrit-Comment-Date: Sat, 03 Aug 2019 22:39:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for National Instruments USB-845x devices
by Miklós Márton (Code Review) 03 Aug '19

03 Aug '19

Miklós Márton has uploaded a new patch set (#22) to the change originally created by David Hendricks. ( https://review.coreboot.org/c/flashrom/+/25683 ) Change subject: Add support for National Instruments USB-845x devices ...................................................................... Add support for National Instruments USB-845x devices Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Signed-off-by: Miklós Márton <martonmiklosqdev(a)gmail.com> --- M Makefile M flashrom.8.tmpl M flashrom.c M libflashrom.c A ni845x_spi.c M programmer.h 6 files changed, 657 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/25683/22 -- To view, visit https://review.coreboot.org/c/flashrom/+/25683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 22 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com> Gerrit-CC: Stefan T <stefan.tauner(a)gmx.at> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: Fix -Wunused-parameter issues
by Angel Pons (Code Review) 03 Aug '19

03 Aug '19

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30410 ) Change subject: Fix -Wunused-parameter issues ...................................................................... Patch Set 5: Code-Review+1 > Patch Set 5: > > > I personally think "(void)flash;" looks really ugly > > It really does. > > Though, I try not to judge it by asthetics but the potential > benefit of the warning. Does anybody recall or imagine bugs > that would be prevented by -Wunused-parameter? Or maybe hide the ugliness behind a macro... (please don't shoot me for suggesting that) -- To view, visit https://review.coreboot.org/c/flashrom/+/30410 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If831398c91e9f49bd5216a03ed4caaa2a7ab02bd Gerrit-Change-Number: 30410 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Richard Hughes <richard(a)hughsie.com> Gerrit-Reviewer: Stefan T <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Sat, 03 Aug 2019 21:43:41 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Makefile,meson.build: Enable/assume -Wextra
by Angel Pons (Code Review) 03 Aug '19

03 Aug '19

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30411 ) Change subject: Makefile,meson.build: Enable/assume -Wextra ...................................................................... Patch Set 5: Code-Review+1 Not sure if all dependencies for this change are met -- To view, visit https://review.coreboot.org/c/flashrom/+/30411 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id2ece264c2d483e34019985dd3a7631c4889abe6 Gerrit-Change-Number: 30411 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Sat, 03 Aug 2019 21:42:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for continuous SPI reads
by Edward O'Callaghan (Code Review) 03 Aug '19

03 Aug '19

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/34584 ) Change subject: Add support for continuous SPI reads ...................................................................... Add support for continuous SPI reads Forward port the original patch from Duncan Laurie <dlaurie(a)chromium.org> in commit 06ffd5263892061c5ebf46e52b7878786cf2cece so that it fits for upstream consumption. This patch adds support for "unbounded reads" which are for continous reads to a SPI flash chip instead of reading in page size blocks. This speeds up the flash process over FTDI by quite a bit, testing the read of a 16MB part it goes from 2m11s to 0m15s. Change-Id: I804545aeb1b827ffdd41b21024fd618475e8263a Signed-off-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M chipdrivers.h M flash.h M spi.c M spi25.c 4 files changed, 40 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/84/34584/1 diff --git a/chipdrivers.h b/chipdrivers.h index e380878..77e1943 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -55,6 +55,7 @@ int spi_chip_write_1(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); int spi_nbyte_read(struct flashctx *flash, unsigned int addr, uint8_t *bytes, unsigned int len); int spi_read_chunked(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len, unsigned int chunksize); +int spi_read_unbound(struct flashchip *flash, uint8_t *buf, unsigned int start, unsigned int len, unsigned int chunksize); int spi_write_chunked(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len, unsigned int chunksize); int spi_enter_4ba(struct flashctx *flash); int spi_exit_4ba(struct flashctx *flash); diff --git a/flash.h b/flash.h index b60a980..981f88b 100644 --- a/flash.h +++ b/flash.h @@ -143,6 +143,8 @@ #define ERASED_VALUE(flash) (((flash)->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff) +#define FEATURE_UNBOUND_READ (1 << 19) + enum test_state { OK = 0, NT = 1, /* Not tested */ diff --git a/spi.c b/spi.c index 489baf3..23f4cfe 100644 --- a/spi.c +++ b/spi.c @@ -75,13 +75,18 @@ unsigned int len) { unsigned int max_data = flash->mst->spi.max_data_read; + int rc; if (max_data == MAX_DATA_UNSPECIFIED) { msg_perr("%s called, but SPI read chunk size not defined " "on this hardware. Please report a bug at " "flashrom(a)flashrom.org\n", __func__); return 1; } - return spi_read_chunked(flash, buf, start, len, max_data); + if (flash->feature_bits & FEATURE_UNBOUND_READ) + rc = spi_read_unbound(flash, buf, start, len, max_data); + else + rc = spi_read_chunked(flash, buf, start, len, max_data); + return rc; } int default_spi_write_256(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) diff --git a/spi25.c b/spi25.c index 2a1d492..3327110 100644 --- a/spi25.c +++ b/spi25.c @@ -659,6 +659,37 @@ } /* + * Read a part of the flash chip. + * Ignore pages and read the data continuously, the only bound is the chunksize. + */ +int spi_read_unbound(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len, unsigned int chunksize) +{ + int rc = 0; + unsigned int i; + + for (i = start; i < (start + len); i += chunksize) { + int chunk_status = 0; + unsigned int toread = min(chunksize, start + len - i); + + chunk_status = spi_nbyte_read(flash, i, buf + (i - start), toread); + if (chunk_status) { + if (ignore_error(chunk_status)) { + /* fill this chunk with 0xff bytes and + let caller know about the error */ + memset(buf + (i - start), 0xff, toread); + rc = chunk_status; + continue; + } else { + rc = chunk_status; + break; + } + } + } + + return rc; +} + +/* * Write a part of the flash chip. * FIXME: Use the chunk code from Michael Karcher instead. * Each page is written separately in chunks with a maximum size of chunksize. -- To view, visit https://review.coreboot.org/c/flashrom/+/34584 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I804545aeb1b827ffdd41b21024fd618475e8263a Gerrit-Change-Number: 34584 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

2 4

Change in flashrom[master]: flashchips.c: Mark AMD Am29F010A/B as TEST_OK_PRE,
by Alan Green (Code Review) 03 Aug '19

03 Aug '19

Alan Green has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/34534 ) Change subject: flashchips.c: Mark AMD Am29F010A/B as TEST_OK_PRE, ...................................................................... flashchips.c: Mark AMD Am29F010A/B as TEST_OK_PRE, The AMD Am29F010 was marked TEST_OK_PRE in chromium repo change SHA d217d1219ccaa43a01cd75475409183bd5714410. There are no other differences in the definition of this chip. This is the only change from the Chromium repo to be upstreamed for AMD chips. Signed-off-by: Alan Green <avg(a)google.com> Change-Id: I7fa10d33b42c09d035c611535a54592083c4eaa0 --- M flashchips.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/34/34534/1 diff --git a/flashchips.c b/flashchips.c index b859bf6..15c33c1 100644 --- a/flashchips.c +++ b/flashchips.c @@ -156,7 +156,7 @@ .total_size = 128, .page_size = 16 * 1024, .feature_bits = FEATURE_ADDR_2AA | FEATURE_EITHER_RESET, - .tested = TEST_UNTESTED, + .tested = TEST_OK_PRE, .probe = probe_jedec, .probe_timing = TIMING_ZERO, .block_erasers = -- To view, visit https://review.coreboot.org/c/flashrom/+/34534 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7fa10d33b42c09d035c611535a54592083c4eaa0 Gerrit-Change-Number: 34534 Gerrit-PatchSet: 1 Gerrit-Owner: Alan Green <avg(a)google.com> Gerrit-MessageType: newchange

4 5

Change in flashrom[master]: flashchips.c: Mark Intel 82802AB as TEST_OK_PREW
by Alan Green (Code Review) 03 Aug '19

03 Aug '19

Alan Green has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/34533 ) Change subject: flashchips.c: Mark Intel 82802AB as TEST_OK_PREW ...................................................................... flashchips.c: Mark Intel 82802AB as TEST_OK_PREW Intel 82802AB Was marked as TEST_OK_PREW in the Chromium fork in their SHA312d9ff1fb1ccb5533a867d4248eb1be95ec3fbc. The definitions in the fork and here in upstream are otherwise substantially similar. Signed-off-by: Alan Green <avg(a)google.com> Change-Id: Iec75f0b1c35000308601fa6fdd63ab1738d0ef94 --- M flashchips.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/34533/1 diff --git a/flashchips.c b/flashchips.c index 166af6a..b859bf6 100644 --- a/flashchips.c +++ b/flashchips.c @@ -7671,7 +7671,7 @@ .total_size = 512, .page_size = 64 * 1024, .feature_bits = FEATURE_REGISTERMAP, - .tested = TEST_OK_PR, + .tested = TEST_OK_PREW, .probe = probe_82802ab, .probe_timing = TIMING_IGNORED, /* routine does not use probe_timing (82802ab.c) */ .block_erasers = -- To view, visit https://review.coreboot.org/c/flashrom/+/34533 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iec75f0b1c35000308601fa6fdd63ab1738d0ef94 Gerrit-Change-Number: 34533 Gerrit-PatchSet: 1 Gerrit-Owner: Alan Green <avg(a)google.com> Gerrit-MessageType: newchange

4 4

Change in ...flashrom[master]: Add support for security register and SBULK command on Macronix chips
by vl_sk (Code Review) 03 Aug '19

03 Aug '19

vl_sk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/32034 Change subject: Add support for security register and SBULK command on Macronix chips ...................................................................... Add support for security register and SBULK command on Macronix chips Signed-off-by: Vladimir Skokov <vl_sk(a)mail.ru> Change-Id: I6b3d7bd2625dec5fda7b85f421d40c49bbc528a6 --- M chipdrivers.h M flashchips.c M spi.h M spi25_statusreg.c 4 files changed, 78 insertions(+), 24 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/34/32034/1 diff --git a/chipdrivers.h b/chipdrivers.h index 134ed26..54288cd 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -102,7 +102,9 @@ int spi_prettyprint_status_register_sst25(struct flashctx *flash); int spi_prettyprint_status_register_sst25vf016(struct flashctx *flash); int spi_prettyprint_status_register_sst25vf040b(struct flashctx *flash); +int spi_prettyprint_security_register_mx25(struct flashctx *flash); int spi_disable_blockprotect_sst26_global_unprotect(struct flashctx *flash); +int spi_disable_blockprotect_mx25_global_unprotect(struct flashctx *flash); /* sfdp.c */ int probe_spi_sfdp(struct flashctx *flash); diff --git a/flashchips.c b/flashchips.c index d85ea74..2c662e9 100644 --- a/flashchips.c +++ b/flashchips.c @@ -8555,9 +8555,8 @@ .block_erase = spi_block_erase_c7, } }, - /* TODO: security register and SBLK/SBULK; MX25L12835F: configuration register */ - .printlock = spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */ - .unlock = spi_disable_blockprotect_bp3_srwd, + .printlock = spi_prettyprint_security_register_mx25, + .unlock = spi_disable_blockprotect_mx25_global_unprotect, .write = spi_chip_write_256, .read = spi_chip_read, /* Fast read (0x0B) supported */ .voltage = {2700, 3600}, @@ -8604,9 +8603,8 @@ .block_erase = spi_block_erase_c7, } }, - /* TODO: security register and SBLK/SBULK; MX25L12835F: configuration register */ - .printlock = spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */ - .unlock = spi_disable_blockprotect_bp3_srwd, + .printlock = spi_prettyprint_security_register_mx25, + .unlock = spi_disable_blockprotect_mx25_global_unprotect, .write = spi_chip_write_256, .read = spi_chip_read, /* Fast read (0x0B) supported */ .voltage = {2700, 3600}, @@ -8693,9 +8691,8 @@ .block_erase = spi_block_erase_c7, } }, - /* TODO: security register */ - .printlock = spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */ - .unlock = spi_disable_blockprotect_bp3_srwd, + .printlock = spi_prettyprint_security_register_mx25, + .unlock = spi_disable_blockprotect_mx25_global_unprotect, .write = spi_chip_write_256, .read = spi_chip_read, /* Fast read (0x0B) and multi I/O supported */ .voltage = {1650, 2000}, @@ -8734,9 +8731,8 @@ .block_erase = spi_block_erase_c7, } }, - /* TODO: security register */ - .printlock = spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */ - .unlock = spi_disable_blockprotect_bp3_srwd, + .printlock = spi_prettyprint_security_register_mx25, + .unlock = spi_disable_blockprotect_mx25_global_unprotect, .write = spi_chip_write_256, .read = spi_chip_read, /* Fast read (0x0B) and multi I/O supported */ .voltage = {1650, 2000}, @@ -8776,9 +8772,8 @@ .block_erase = spi_block_erase_c7, } }, - /* TODO: security register */ - .printlock = spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */ - .unlock = spi_disable_blockprotect_bp3_srwd, + .printlock = spi_prettyprint_security_register_mx25, + .unlock = spi_disable_blockprotect_mx25_global_unprotect, .write = spi_chip_write_256, .read = spi_chip_read, /* Fast read (0x0B) and multi I/O supported */ .voltage = {1650, 2000}, @@ -8818,9 +8813,8 @@ .block_erase = spi_block_erase_c7, } }, - /* TODO: security register */ - .printlock = spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */ - .unlock = spi_disable_blockprotect_bp3_srwd, + .printlock = spi_prettyprint_security_register_mx25, + .unlock = spi_disable_blockprotect_mx25_global_unprotect, .write = spi_chip_write_256, .read = spi_chip_read, /* Fast read (0x0B) and multi I/O supported */ .voltage = {1650, 2000}, @@ -8858,9 +8852,8 @@ .block_erase = spi_block_erase_c7, } }, - /* TODO: security register */ - .printlock = spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */ - .unlock = spi_disable_blockprotect_bp3_srwd, + .printlock = spi_prettyprint_security_register_mx25, + .unlock = spi_disable_blockprotect_mx25_global_unprotect, .write = spi_chip_write_256, /* Multi I/O supported */ .read = spi_chip_read, /* Fast read (0x0B) and multi I/O supported */ .voltage = {1650, 2000}, @@ -8907,9 +8900,8 @@ .block_erase = spi_block_erase_c7, } }, - /* TODO: security register */ - .printlock = spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */ - .unlock = spi_disable_blockprotect_bp3_srwd, + .printlock = spi_prettyprint_security_register_mx25, + .unlock = spi_disable_blockprotect_mx25_global_unprotect, .write = spi_chip_write_256, /* Multi I/O supported */ .read = spi_chip_read, /* Fast read (0x0B) and multi I/O supported */ .voltage = {1650, 2000}, diff --git a/spi.h b/spi.h index 422e6a0..456d894 100644 --- a/spi.h +++ b/spi.h @@ -170,6 +170,15 @@ From ANY mode (3-bytes or 4-bytes) it works with 4-byte address */ #define JEDEC_BYTE_PROGRAM_4BA 0x12 +/* Read Security Register */ +#define JEDEC_RDSCUR 0x2B +#define JEDEC_RDSCUR_OUTSIZE 0x01 +#define JEDEC_RDSCUR_INSIZE 0x01 + +/* Do GBULK (gang block unlock) */ +#define JEDEC_GBULK 0x98 +#define JEDEC_GBULK_OUTSIZE 0x01 + /* Error codes */ #define SPI_GENERIC_ERROR -1 #define SPI_INVALID_OPCODE -2 diff --git a/spi25_statusreg.c b/spi25_statusreg.c index 4cf7023..4cdf800 100644 --- a/spi25_statusreg.c +++ b/spi25_statusreg.c @@ -237,6 +237,28 @@ return spi_disable_blockprotect_generic(flash, 0x7C, 1 << 7, 0, 0xFF); } +int spi_disable_blockprotect_mx25_global_unprotect(struct flashctx *flash) +{ + static const unsigned char cmd[JEDEC_GBULK_OUTSIZE] = { JEDEC_GBULK }; + int ret; + + ret = spi_write_enable(flash); + if (ret) { + msg_cerr("Set WREN failed!\n"); + return -1; + } + + /* Send GBULK cmd to device. */ + ret = spi_send_command(flash, sizeof(cmd), 0, cmd, NULL); + if (ret) { + msg_cerr("GBULK failed!\n"); + return -1; + } + + /* Disable standard blockprotect */ + return spi_disable_blockprotect_bp3_srwd(flash); +} + static void spi_prettyprint_status_register_hex(uint8_t status) { msg_cdbg("Chip status register is 0x%02x.\n", status); @@ -751,3 +773,32 @@ msg_cdbg("Resulting block protection : %s\n", bpt[(status & 0x1c) >> 2]); return 0; } + +/* === Macronix chips with Security Register === */ + +int spi_read_security_register(struct flashctx *flash) +{ + static const unsigned char cmd[JEDEC_RDSCUR_OUTSIZE] = { JEDEC_RDSCUR }; + unsigned char readarr[JEDEC_RDSCUR_INSIZE]; + int ret; + + ret = spi_send_command(flash, sizeof(cmd), sizeof(readarr), cmd, readarr); + if (ret) { + msg_cerr("RDSCUR failed!\n"); + return 0; + } + + return readarr[0]; +} + +int spi_prettyprint_security_register_mx25(struct flashctx *flash) +{ + int security_register = spi_read_security_register(flash); + int wpsel = !!(security_register & (1 << 7)); + + msg_cdbg("Chip security register: 0x%02X\n", security_register); + msg_cdbg("\tWPSEL is %s\n", wpsel ? "ON" : "OFF"); + + /* Print standard blockprotect info */ + return spi_prettyprint_status_register_bp3_srwd(flash); +} \ No newline at end of file -- To view, visit https://review.coreboot.org/c/flashrom/+/32034 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6b3d7bd2625dec5fda7b85f421d40c49bbc528a6 Gerrit-Change-Number: 32034 Gerrit-PatchSet: 1 Gerrit-Owner: vl_sk Gerrit-MessageType: newchange

3 5

Change in flashrom[master]: Add support for National Instruments USB-845x devices
by Nico Huber (Code Review) 03 Aug '19

03 Aug '19

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/25683 ) Change subject: Add support for National Instruments USB-845x devices ...................................................................... Patch Set 21: (10 comments) Beside usb8452_spi_set_io_voltage() and some nits, this looks quite good :) But indentation seems to be broken in many places now (more than I commented on). It seems your editor assumes 1 tab == 4 character spaces? it should be 8. https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c File ni845x_spi.c: https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@150 PS21, Line 150: "USB%u::0x%04X::0x%04X::%08lX::RAW", Nit, odd indentation (it seems you replaced 8 spaces with 2 tabs? should be 1). https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@151 PS21, Line 151: &usb_bus, &vid, &pid, &serial_as_number) != 4) { here too https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@154 PS21, Line 154: resource_name); and here https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@222 PS21, Line 222: // usb8452_io_voltages_in_100mV is a decreasing list of supported voltages comment seems already outdated :-( https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@227 PS21, Line 227: i + 1 This condition is far too complex, and, if I read this correctly, it may access beyond the array if the requested voltage is above all array entries. i.e. for `i == ARRAY_SIZE() - 1` and `IO_voltage_100mV > usb8452_io_voltages_in_100mV[i]` this would be: if ((true && false) || (true && undefined)) https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@222 PS21, Line 222: // usb8452_io_voltages_in_100mV is a decreasing list of supported voltages : for (i = 0; i < ARRAY_SIZE(usb8452_io_voltages_in_100mV); i++) { : if ((i == (ARRAY_SIZE(usb8452_io_voltages_in_100mV) - 1) && : usb8452_io_voltages_in_100mV[i] == IO_voltage_100mV) || : (usb8452_io_voltages_in_100mV[i] <= IO_voltage_100mV && : IO_voltage_100mV < usb8452_io_voltages_in_100mV[i + 1])) { : selected_voltage_100mV = usb8452_io_voltages_in_100mV[i]; : if (IO_voltage_100mV != usb8452_io_voltages_in_100mV[i]) { : msg_pwarn("USB-8452 IO Voltage forced to: %.1f V\n", : (float)selected_voltage_100mV / 10.0f); : } else { : msg_pinfo("USB-8452 IO Voltage set to: %.1f V\n", : (float)selected_voltage_100mV / 10.0f); : } : break; : } : } : : if (selected_voltage_100mV == 0) { : msg_pwarn("The USB-8452 does not supports the %.1f V IO voltage\n", : IOVoltage_mV / 10.0f); : selected_voltage_100mV = kNi845x12Volts; : msg_pwarn("The output voltage is set to 1.2V (this is the lowest voltage)\n"); : msg_pwarn("Supported IO voltages: "); : for (i = 0; i < ARRAY_SIZE(usb8452_io_voltages_in_100mV); i--) { : msg_pwarn("%.1fV", (float)usb8452_io_voltages_in_100mV[i] / 10.0f); : if (i != ARRAY_SIZE(usb8452_io_voltages_in_100mV) - 1) : msg_pwarn(", "); : } : msg_pwarn("\n"); : return -1; : } I think this whole block could benefit from a better separation of selecting the array index and status reporting, e.g. for an ascending list, this should be all that's needed to select the index (it should be that easy, because we know the list is ordered): for (i = ARRAY_SIZE(usb8452_io_voltages_in_100mV); i > 0; --i) { if (IO_voltage_100mV >= usb8452_io_voltages_in_100mV[i]) break; } selected_voltage_100mV = usb8452_io_voltages_in_100mV[i]; Then, we could decide what message to show: if (IO_voltage_100mV < usb8452_io_voltages_in_100mV[0]) { /* unsupported / would have to round up */ unsupported voltage message voltages list return -1; } else if (selected_voltage_100mV != IO_voltage_100mV) { /* we rounded down */ msg_pwarn("USB-8452 IO Voltage forced to: %.1f V\n", ...); } else { /* exact match */ msg_pinfo("USB-8452 IO Voltage set to: %.1f V\n", ...); } Or maybe even move `if (IO_voltage_100mV < usb8452_io_voltages_in_100mV[0])` above the loop? Now that I looked up it, would actually match well with the `if (IOVoltage_mV > 3300)`. And why would we print the whole list of sup- ported voltages only for values below but not above the possible? https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@270 PS21, Line 270: int32 i = ni845xSpiConfigurationSetClockRate(configuration_handle, SCK_freq_in_KHz); broken indentation https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@285 PS21, Line 285: if (clock_freq_read_KHz != SCK_freq_in_KHz) { : msg_pinfo("SPI clock frequency forced to: %d KHz (requested: %d KHz)\n", : (int)clock_freq_read_KHz, (int)SCK_freq_in_KHz); : } else { : msg_pinfo("SPI clock frequency set to: %d KHz\n", (int)SCK_freq_in_KHz); : } : return 0; indentation https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@365 PS21, Line 365: if (CS_number < 0 || 7 < CS_number) { Missing check of length of `CS_str`, e.g. what happens if somebody passes "10"? https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@386 PS21, Line 386: free(speed_str); move above the `if`? -- To view, visit https://review.coreboot.org/c/flashrom/+/25683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 21 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com> Gerrit-CC: Stefan T <stefan.tauner(a)gmx.at> Gerrit-Comment-Date: Sat, 03 Aug 2019 13:32:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit August 2019