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

Change in ...flashrom[master]: dediprog: Allow 4BA on SF100
by Patrick Rudolph (Code Review) June 6, 2019

June 6, 2019

Hello Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/33195 to review the following change. Change subject: dediprog: Allow 4BA on SF100 ...................................................................... dediprog: Allow 4BA on SF100 Tested on dediprog SF100. Change-Id: I8822b79f46876feff0fd443f711c57dffb67b349 Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com> --- M dediprog.c 1 file changed, 2 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/95/33195/1 diff --git a/dediprog.c b/dediprog.c index 03675e5..3564f6f 100644 --- a/dediprog.c +++ b/dediprog.c @@ -1134,7 +1134,8 @@ if (dediprog_standalone_mode()) return 1; - if (dediprog_devicetype == DEV_SF600 && protocol() == PROTOCOL_V2) + if ((dediprog_devicetype == DEV_SF600 || dediprog_devicetype == DEV_SF100) && + protocol() == PROTOCOL_V2) spi_master_dediprog.features |= SPI_MASTER_4BA; if (register_spi_master(&spi_master_dediprog) || dediprog_set_leds(LED_NONE)) -- To view, visit https://review.coreboot.org/c/flashrom/+/33195 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8822b79f46876feff0fd443f711c57dffb67b349 Gerrit-Change-Number: 33195 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

3 9

Change in ...flashrom[master]: Rework internal bus handling and laptop bail-out
by Thomas Heijligen (Code Review) June 6, 2019

June 6, 2019

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/28716 ) Change subject: Rework internal bus handling and laptop bail-out ...................................................................... Patch Set 7: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/28716 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1dbda8cf0c10d7786106f14f0d18c3dcce35f0a3 Gerrit-Change-Number: 28716 Gerrit-PatchSet: 7 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: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 06 Jun 2019 12:14:12 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: Rework internal bus handling and laptop bail-out
by Paul Menzel (Code Review) June 6, 2019

June 6, 2019

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/28716 ) Change subject: Rework internal bus handling and laptop bail-out ...................................................................... Patch Set 7: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/28716 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1dbda8cf0c10d7786106f14f0d18c3dcce35f0a3 Gerrit-Change-Number: 28716 Gerrit-PatchSet: 7 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: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 06 Jun 2019 09:37:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: Rework internal bus handling and laptop bail-out
by Nico Huber (Code Review) June 5, 2019

June 5, 2019

Hello Angel Pons, Arthur Heymans, Matt DeVillier, Thomas Heijligen, David Hendricks, Paul Menzel, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/28716 to look at the new patch set (#7). Change subject: Rework internal bus handling and laptop bail-out ...................................................................... Rework internal bus handling and laptop bail-out We used to bail out on any unknown laptop. However, modern systems with SPI flashes don't suffer from the original problem. Even if a flash chip is shared with the EC, the latter has to expect the host to send regular JEDEC SPI commands any time. So instead of bailing out, we limit the set of buses to probe. If we suspect to be running on a laptop, we only allow probing of SPI and opaque programmers. The user can still use the existing force options to probe all buses. This will obsolete some board-enables that could be moved to `print.c` in follow-up commits. Change-Id: I1dbda8cf0c10d7786106f14f0d18c3dcce35f0a3 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M board_enable.c M chipset_enable.c M flashrom.8.tmpl M ichspi.c M internal.c M programmer.h 6 files changed, 500 insertions(+), 424 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/16/28716/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/28716 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1dbda8cf0c10d7786106f14f0d18c3dcce35f0a3 Gerrit-Change-Number: 28716 Gerrit-PatchSet: 7 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: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in ...flashrom[master]: Rework internal bus handling and laptop bail-out
by Thomas Heijligen (Code Review) June 5, 2019

June 5, 2019

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/28716 ) Change subject: Rework internal bus handling and laptop bail-out ...................................................................... Patch Set 6: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/28716 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1dbda8cf0c10d7786106f14f0d18c3dcce35f0a3 Gerrit-Change-Number: 28716 Gerrit-PatchSet: 6 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: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 05 Jun 2019 11:21:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: Rework internal bus handling and laptop bail-out
by Nico Huber (Code Review) June 5, 2019

June 5, 2019

Hello Angel Pons, Arthur Heymans, Matt DeVillier, David Hendricks, Paul Menzel, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/28716 to look at the new patch set (#6). Change subject: Rework internal bus handling and laptop bail-out ...................................................................... Rework internal bus handling and laptop bail-out We used to bail out on any unknown laptop. However, modern systems with SPI flashes don't suffer from the original problem. Even if a flash chip is shared with the EC, the latter has to expect the host to send regular JEDEC SPI commands any time. So instead of bailing out, we limit the set of buses to probe. If we suspect to be running on a laptop, we only allow probing of SPI and opaque programmers. The user can still use the existing force options to probe all buses. This will obsolete some board-enables that could be moved to `print.c` in follow-up commits. Change-Id: I1dbda8cf0c10d7786106f14f0d18c3dcce35f0a3 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M board_enable.c M chipset_enable.c M flashrom.8.tmpl M ichspi.c M internal.c M programmer.h 6 files changed, 500 insertions(+), 424 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/16/28716/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/28716 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1dbda8cf0c10d7786106f14f0d18c3dcce35f0a3 Gerrit-Change-Number: 28716 Gerrit-PatchSet: 6 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: Matt DeVillier <matt.devillier(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: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newpatchset

1 0

Change in ...flashrom[master]: Rework internal bus handling and laptop bail-out
by Thomas Heijligen (Code Review) June 4, 2019

June 4, 2019

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/28716 ) Change subject: Rework internal bus handling and laptop bail-out ...................................................................... Patch Set 5: please adapt documentation (man page) - remove forece_I_want_to_brick - add hint to iomem=relaxed -- To view, visit https://review.coreboot.org/c/flashrom/+/28716 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1dbda8cf0c10d7786106f14f0d18c3dcce35f0a3 Gerrit-Change-Number: 28716 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: Matt DeVillier <matt.devillier(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: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Tue, 04 Jun 2019 20:55:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: Rework internal bus handling and laptop bail-out
by Arthur Heymans (Code Review) June 4, 2019

June 4, 2019

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/28716 ) Change subject: Rework internal bus handling and laptop bail-out ...................................................................... Patch Set 5: Code-Review+1 manpage -- To view, visit https://review.coreboot.org/c/flashrom/+/28716 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1dbda8cf0c10d7786106f14f0d18c3dcce35f0a3 Gerrit-Change-Number: 28716 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: Matt DeVillier <matt.devillier(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-Comment-Date: Tue, 04 Jun 2019 18:07:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: dediprog: Implement 4BA EAR mode for protocol v1
by Nico Huber (Code Review) June 4, 2019

June 4, 2019

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/32016 Change subject: dediprog: Implement 4BA EAR mode for protocol v1 ...................................................................... dediprog: Implement 4BA EAR mode for protocol v1 With an SF100 and protocol version 1, using the extended address register of the flash chip seems safe. Make use of that and remove the broken 4BA modes flag. Tested with SF100 V:5.1.9 and W25Q256FV. Change-Id: If926cf3cbbebf88231116c4d65bafc19d23646f6 Signed-off-by: Nico Huber <nico.huber(a)secunet.com> --- M chipdrivers.h M dediprog.c M spi25.c 3 files changed, 26 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/16/32016/1 diff --git a/chipdrivers.h b/chipdrivers.h index 134ed26..e380878 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -58,6 +58,7 @@ 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); +int spi_set_extended_address(struct flashctx *, uint8_t addr_high); /* spi25_statusreg.c */ diff --git a/dediprog.c b/dediprog.c index 03675e5..d48c541 100644 --- a/dediprog.c +++ b/dediprog.c @@ -360,7 +360,7 @@ return 0; } -static void prepare_rw_cmd( +static int prepare_rw_cmd( struct flashctx *const flash, uint8_t *data_packet, unsigned int count, uint8_t dedi_spi_cmd, unsigned int *value, unsigned int *idx, unsigned int start, int is_read) { /* First 5 bytes are common in both generations. */ @@ -400,9 +400,23 @@ } } } else { - *value = start % 0x10000; - *idx = start / 0x10000; + if (flash->chip->feature_bits & FEATURE_4BA_EXT_ADDR) { + if (spi_set_extended_address(flash, start >> 24)) + return 1; + } else if (start >> 24) { + msg_cerr("Can't handle 4-byte address with dediprog.\n"); + return 1; + } + /* + * We don't know how the dediprog firmware handles 4-byte + * addresses. So let's not tell it what we are doing and + * only send the lower 3 bytes. + */ + *value = start & 0xffff; + *idx = (start >> 16) & 0xff; } + + return 0; } /* Bulk read interface, will read multiple 512 byte chunks aligned to 512 bytes. @@ -448,7 +462,8 @@ uint8_t data_packet[command_packet_size]; unsigned int value, idx; - prepare_rw_cmd(flash, data_packet, count, READ_MODE_STD, &value, &idx, start, 1); + if (prepare_rw_cmd(flash, data_packet, count, READ_MODE_STD, &value, &idx, start, 1)) + return 1; int ret = dediprog_write(CMD_READ, value, idx, data_packet, sizeof(data_packet)); if (ret != sizeof(data_packet)) { @@ -604,7 +619,8 @@ uint8_t data_packet[command_packet_size]; unsigned int value, idx; - prepare_rw_cmd(flash, data_packet, count, dedi_spi_cmd, &value, &idx, start, 0); + if (prepare_rw_cmd(flash, data_packet, count, dedi_spi_cmd, &value, &idx, start, 0)) + return 1; int ret = dediprog_write(CMD_WRITE, value, idx, data_packet, sizeof(data_packet)); if (ret != sizeof(data_packet)) { msg_perr("Command Write SPI Bulk failed, %s!\n", libusb_error_name(ret)); @@ -1134,6 +1150,9 @@ if (dediprog_standalone_mode()) return 1; + if (dediprog_devicetype == DEV_SF100 && protocol() == PROTOCOL_V1) + spi_master_dediprog.features &= ~SPI_MASTER_NO_4BA_MODES; + if (dediprog_devicetype == DEV_SF600 && protocol() == PROTOCOL_V2) spi_master_dediprog.features |= SPI_MASTER_4BA; diff --git a/spi25.c b/spi25.c index 8b6c462..fd87dc9 100644 --- a/spi25.c +++ b/spi25.c @@ -363,7 +363,7 @@ return result; } -static int spi_set_extended_address(struct flashctx *const flash, const uint8_t addr_high) +int spi_set_extended_address(struct flashctx *const flash, const uint8_t addr_high) { if (flash->address_high_byte != addr_high && spi_write_extended_address_register(flash, addr_high)) -- To view, visit https://review.coreboot.org/c/flashrom/+/32016 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If926cf3cbbebf88231116c4d65bafc19d23646f6 Gerrit-Change-Number: 32016 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: newchange

2 2

Change in ...flashrom[master]: Rework internal bus handling and laptop bail-out
by Arthur Heymans (Code Review) June 4, 2019

June 4, 2019

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/28716 ) Change subject: Rework internal bus handling and laptop bail-out ...................................................................... Patch Set 5: Code-Review+2 (1 comment) Looks fine https://review.coreboot.org/#/c/28716/5/internal.c File internal.c: https://review.coreboot.org/#/c/28716/5/internal.c@285 PS5, Line 285: is_laptop && !(laptop_ok Could we have enums for is_laptop and laptop_ok? -- To view, visit https://review.coreboot.org/c/flashrom/+/28716 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1dbda8cf0c10d7786106f14f0d18c3dcce35f0a3 Gerrit-Change-Number: 28716 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: Matt DeVillier <matt.devillier(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-Comment-Date: Mon, 03 Jun 2019 22:41:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0