- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: edi: Print debug info like others while probing for ENE chips
by Mike Banon (Code Review) 06 Feb '18

06 Feb '18

Mike Banon has posted comments on this change. ( https://review.coreboot.org/23261 ) Change subject: edi: Print debug info like others while probing for ENE chips ...................................................................... Patch Set 6: (1 comment) https://review.coreboot.org/#/c/23261/5/edi.c File edi.c: https://review.coreboot.org/#/c/23261/5/edi.c@164 PS5, Line 164: if (chip->hwversion == hwversion && chip->ediid == ediid) { > After a closer look, it looks like this is not consistent with the way it's done for spi25 chips. […] Originally it was msg_cdbg but I changed it to msg_cdbg2 after Nico's request. Do you think its a good idea to change it back to msg_cdbg ? Also, right now if hwversion and/or ediid do not match these values , the exit code is 0 - while, if they match, the exit code is 1. If we replace it with a single msg line, what exit code should it be? "return 1" for both cases? -- To view, visit https://review.coreboot.org/23261 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8e62bc9f6785b4bf0be0aaf0f74c8120d77c0d4 Gerrit-Change-Number: 23261 Gerrit-PatchSet: 6 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Kocialkowski <contact(a)paulk.fr> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 05 Feb 2018 22:31:13 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: edi: Print debug info like others while probing for ENE chips
by build bot (Jenkins) (Code Review) 06 Feb '18

06 Feb '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23261 ) Change subject: edi: Print debug info like others while probing for ENE chips ...................................................................... Patch Set 6: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/1103/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/932/ : SUCCESS -- To view, visit https://review.coreboot.org/23261 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8e62bc9f6785b4bf0be0aaf0f74c8120d77c0d4 Gerrit-Change-Number: 23261 Gerrit-PatchSet: 6 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Kocialkowski <contact(a)paulk.fr> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 05 Feb 2018 22:25:52 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: edi: Print debug info like others while probing for ENE chips
by Mike Banon (Code Review) 06 Feb '18

06 Feb '18

Hello Paul Kocialkowski, Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/23261 to look at the new patch set (#6). Change subject: edi: Print debug info like others while probing for ENE chips ...................................................................... edi: Print debug info like others while probing for ENE chips Instead of just "Probing for ENE KB9012 (EDI), 128 kB:", lets print some debug info - like it is currently being printed for other chips: Probing for ENE KB9012 (EDI), 128 kB: edi_chip_probe: hwversion 0xc3, ediid 0x04 Found ENE flash chip "KB9012 (EDI)" (128 kB, SPI) on ch341a_spi. Change-Id: Id8e62bc9f6785b4bf0be0aaf0f74c8120d77c0d4 Signed-off-by: Mike Banon <mikebdp2(a)gmail.com> Signed-off-by: Paul Kocialkowski <contact(a)paulk.fr> --- M edi.c 1 file changed, 12 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/61/23261/6 -- To view, visit https://review.coreboot.org/23261 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id8e62bc9f6785b4bf0be0aaf0f74c8120d77c0d4 Gerrit-Change-Number: 23261 Gerrit-PatchSet: 6 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Kocialkowski <contact(a)paulk.fr> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: Add support for the ENE Embedded Debug Interface EDI and KB9012 EC
by build bot (Jenkins) (Code Review) 06 Feb '18

06 Feb '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23259 ) Change subject: Add support for the ENE Embedded Debug Interface EDI and KB9012 EC ...................................................................... Patch Set 5: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/1102/ : SUCCESS -- To view, visit https://review.coreboot.org/23259 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib8b2eb2feeef5c337d725d15ebf994a299897854 Gerrit-Change-Number: 23259 Gerrit-PatchSet: 5 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Paul Kocialkowski <contact(a)paulk.fr> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 05 Feb 2018 22:15:04 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: Add support for the ENE Embedded Debug Interface EDI and KB9012 EC
by Mike Banon (Code Review) 06 Feb '18

06 Feb '18

Hello Paul Kocialkowski, Paul Menzel, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/23259 to look at the new patch set (#5). Change subject: Add support for the ENE Embedded Debug Interface EDI and KB9012 EC ...................................................................... Add support for the ENE Embedded Debug Interface EDI and KB9012 EC The ENE Embedded Debug Interface (EDI) is a SPI-based interface for accessing the memory of ENE embedded controllers. The ENE KB9012 EC is an embedded controller found on various laptops such as the Lenovo G505s. It features a 8051 microcontroller and has 128 KiB of internal storage for program data. EDI can be accessed on the KB9012 through pins 59-62 (CS-CLK-MOSI-MISO) when flash direct access is not in use. Some firmwares disable EDI at runtime so it might be necessary to ground pin 42 to reset the 8051 microcontroller before accessing the KB9012 via EDI. The example of flashing KB9012 at Lenovo G505S laptop could be found here: http://dangerousprototypes.com/docs/Flashing_KB9012_with_Bus_Pirate Original patch has been created by Paul Kocialkowski, the previous version: http://patchwork.coreboot.org/patch/4413/ Now it has been ported to the latest source code of flashrom master branch Change-Id: Ib8b2eb2feeef5c337d725d15ebf994a299897854 Signed-off-by: Mike Banon <mikebdp2(a)gmail.com> Signed-off-by: Paul Kocialkowski <contact(a)paulk.fr> --- M Makefile M chipdrivers.h A edi.c A edi.h A ene.h M flashchips.c 6 files changed, 608 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/59/23259/5 -- To view, visit https://review.coreboot.org/23259 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib8b2eb2feeef5c337d725d15ebf994a299897854 Gerrit-Change-Number: 23259 Gerrit-PatchSet: 5 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Paul Kocialkowski <contact(a)paulk.fr> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: Add a SPI command class to `struct flashchip`
by Nico Huber (Code Review) 05 Feb '18

05 Feb '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23262 ) Change subject: Add a SPI command class to `struct flashchip` ...................................................................... Patch Set 5: (1 comment) https://review.coreboot.org/#/c/23262/5/flashrom.c File flashrom.c: https://review.coreboot.org/#/c/23262/5/flashrom.c@1213 PS5, Line 1213: SPI25 Could as well be a parameter to this function. -- To view, visit https://review.coreboot.org/23262 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a53ccaef2791a2ac32904d7ab813da7478a6f0 Gerrit-Change-Number: 23262 Gerrit-PatchSet: 5 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Kocialkowski <contact(a)paulk.fr> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 05 Feb 2018 21:13:37 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add a SPI command class to `struct flashchip`
by Nico Huber (Code Review) 05 Feb '18

05 Feb '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23262 ) Change subject: Add a SPI command class to `struct flashchip` ...................................................................... Patch Set 5: (1 comment) > Note that naming the field spi_command_set breaks the current tab > alignment in the chips declaration, e.g.: > .spi_command_set = EDI, > .probe = edi_probe_kb9012, > > I think naming it "command_set" would be enough since SPI25 already > has SPI in the name and we could generalize the issue with other > protoocls anyway. I would like to keep the `spi_` in the name to make it clear which values to expect. Though, I shortened it to `spi_cmd_set`. > > Also, I think it would be more convenient to be able to specify the > command set to use from the command line instead of specifying the > exact chip. One might want to just hook EDI to the keyboard pins > for reflashing the EC and let flashrom detect which ENE chip it is. Right, I kept that in mind. Should be easy to add to the CLI in a follow up. > > If you want to pick up that idea, that'd be great, otherwise I'll > probably end up writing and submitting something in this direction. Please, go ahead. https://review.coreboot.org/#/c/23262/4/flash.h File flash.h: https://review.coreboot.org/#/c/23262/4/flash.h@186 PS4, Line 186: /* > By the way, it looks like the dummy structure commented out in the flashchips struct array declarati […] It's obviously unmaintained because it documents this struct in a very different place. I wouldn't object to drop it or move it above the definition here. -- To view, visit https://review.coreboot.org/23262 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a53ccaef2791a2ac32904d7ab813da7478a6f0 Gerrit-Change-Number: 23262 Gerrit-PatchSet: 5 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Kocialkowski <contact(a)paulk.fr> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 05 Feb 2018 21:12:43 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add a SPI command class to `struct flashchip`
by build bot (Jenkins) (Code Review) 05 Feb '18

05 Feb '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23262 ) Change subject: Add a SPI command class to `struct flashchip` ...................................................................... Patch Set 5: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/1101/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/931/ : SUCCESS -- To view, visit https://review.coreboot.org/23262 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89a53ccaef2791a2ac32904d7ab813da7478a6f0 Gerrit-Change-Number: 23262 Gerrit-PatchSet: 5 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Kocialkowski <contact(a)paulk.fr> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 05 Feb 2018 21:04:19 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: Add a SPI command class to `struct flashchip`
by Nico Huber (Code Review) 05 Feb '18

05 Feb '18

Nico Huber has uploaded a new patch set (#5) to the change originally created by Mike Banon. ( https://review.coreboot.org/23262 ) Change subject: Add a SPI command class to `struct flashchip` ...................................................................... Add a SPI command class to `struct flashchip` By default, we want to probe for SPI25 chips only. Other SPI use cases, like the ENE/EDI protocol, might use commands that can confuse these common chips. Now, flashrom will probe for a chip only if one of these conditions is true: 1) no chip has been specified AND the chip uses the SPI25 commands 2) this chip has been specified by -c | --chip <chipname> The CLI can later be extended to probe for a specific class of chips. Change-Id: I89a53ccaef2791a2ac32904d7ab813da7478a6f0 Signed-off-by: Mike Banon <mikebdp2(a)gmail.com> Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M flash.h M flashrom.c 2 files changed, 14 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/23262/5 -- To view, visit https://review.coreboot.org/23262 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I89a53ccaef2791a2ac32904d7ab813da7478a6f0 Gerrit-Change-Number: 23262 Gerrit-PatchSet: 5 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Kocialkowski <contact(a)paulk.fr> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: Add support for reading the current flash contents from a file
by Nico Huber (Code Review) 05 Feb '18

05 Feb '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23263 ) Change subject: Add support for reading the current flash contents from a file ...................................................................... Patch Set 6: (4 comments) > Sorry for double patch commits (forgot to add enum the first time) No worries, I'm used to changes that get to 20+ patch-sets before the review even starts ;) and we are making great progress here. https://review.coreboot.org/#/c/23263/6/flashrom.c File flashrom.c: https://review.coreboot.org/#/c/23263/6/flashrom.c@2396 PS6, Line 2396: msg_cinfo("Assumed old flash chip contents as ref-file...\n"); Nit, maybe rather "Assuming ..."? https://review.coreboot.org/#/c/23263/6/flashrom.c@2395 PS6, Line 2395: if (refcontents && oldcontents) { : msg_cinfo("Assumed old flash chip contents as ref-file...\n"); : memcpy(oldcontents, refcontents, flash_size); : memcpy(curcontents, refcontents, flash_size); : } else { Code flow is still not correct, or at least is more limited than it should be. IMO, like this: if (refcontents) { msg_cinfo("Assumed old flash chip contents as ref-file...\n"); memcpy(curcontents, refcontents, flash_size); if (oldcontents) memcpy(oldcontents, refcontents, flash_size); } Please read the whole function again and make sure every possible path combination makes sense. It shouldn't be just me who sees it as correct. https://review.coreboot.org/#/c/23263/6/flashrom.c@2578 PS6, Line 2578: goto _free_ret; Um, I shouldn't have started talking about `const` I guess. I see what you are doing here, and like it, but now you leak the memory behind `refcontents` here. IMO, if you want to use `const` and a common return path, you'd have to move the malloc() up like this: uint8_t *const newcontents = malloc(flash_size); uint8_t *const refcontents = referencefile ? malloc(flash_size) : NULL; if (!newcontents || referencefile && !refcontents) { msg_gerr(...); goto _free_ret; } .... _free_ret: free(refcontents); free(newcontents); return ret; But it'd also be just fine with normal pointer variables that start with NULL and maybe point to something later. https://review.coreboot.org/#/c/23263/6/flashrom.c@2584 PS6, Line 2584: ret = flashrom_image_write(flash, newcontents, flash_size, NULL); Please always use curly braces on all paths of an if/else if you use them on one path (it looks very unbalanced otherwise). -- To view, visit https://review.coreboot.org/23263 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idf153b6955f37779ae9bfb228a434ed10c304947 Gerrit-Change-Number: 23263 Gerrit-PatchSet: 6 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Kocialkowski <contact(a)paulk.fr> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 05 Feb 2018 20:53:43 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit