January 2018 - flashrom-gerrit

Change in flashrom[master]: [v4, 5/6] Add the hidden-from-probing chips category and make KB9012 h...
by Mike Banon (Code Review) Jan. 31, 2018

Jan. 31, 2018

Mike Banon has posted comments on this change. ( https://review.coreboot.org/23262 ) Change subject: [v4,5/6] Add the hidden-from-probing chips category and make KB9012 hidden ...................................................................... Patch Set 1: > (1 comment) > > This should be done before adding the EDI driver. > > AFAIR, Paul had something more flexible in mind: Instead of the > `hidden` flag a grouping of the chips. Basically the same what you > did but instead of 0/1 values a default of JEDEC and alternatively > EDI and whatever might come in the future. How about that? I agree that grouping is a better idea. To be honest I'm not familiar with flashrom codebase, and at this moment it was a much simpler solution to just add this ".hidden" flag, partially because I didn't understand what would be the proper good way of introducing the groups... Are there any other problematic chips supported by flashrom like KB9012, probing for which could cause problems for other chips? If not, maybe it's still possible to use that ".hidden" flag as the quick initial solution (just to avoid KB9012 probing when its not needed) and upgrade it to the groups later, when other "hidden chips" arrive? -- 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: 1 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: Tue, 30 Jan 2018 23:35:19 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: [v4, 4/6] ENE EDI - print debug info like others while probing for ENE...
by build bot (Jenkins) (Code Review) Jan. 31, 2018

Jan. 31, 2018

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23261 ) Change subject: [v4,4/6] ENE EDI - print debug info like others while probing for ENE chips ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/923/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/1083/ : 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: 2 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 Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 30 Jan 2018 23:19:27 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: [v4, 4/6] ENE EDI - print debug info like others while probing for ENE...
by Mike Banon (Code Review) Jan. 31, 2018

Jan. 31, 2018

Mike Banon has posted comments on this change. ( https://review.coreboot.org/23261 ) Change subject: [v4,4/6] ENE EDI - print debug info like others while probing for ENE chips ...................................................................... Patch Set 2: (4 comments) https://review.coreboot.org/#/c/23261/1/edi.c File edi.c: https://review.coreboot.org/#/c/23261/1/edi.c@154 PS1, Line 154: %i < > It's a signed integer, why print it in hex? sorry, my mistake https://review.coreboot.org/#/c/23261/1/edi.c@164 PS1, Line 164: if (chip->hwversion == hwversion && chip->ediid == ediid) { > No need to print them, they are 0 on this path. indeed. fixed https://review.coreboot.org/#/c/23261/1/edi.c@169 PS1, Line 169: __func__, hwversion, chip->hwversion, ediid, chip->ediid); > Please add braces here too (they are not required by syntax but help […] braces added https://review.coreboot.org/#/c/23261/1/edi.c@171 PS1, Line 171: > Actually, I'm not sure if we want this message at all. This function […] changed the debug level to msg_cdbg2, hopefully these debug messages will be useful in some 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: 2 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 Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 30 Jan 2018 23:18:45 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: [v4, 4/6] ENE EDI - print debug info like others while probing for ENE...
by Mike Banon (Code Review) Jan. 31, 2018

Jan. 31, 2018

Hello 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 (#2). Change subject: [v4,4/6] ENE EDI - print debug info like others while probing for ENE chips ...................................................................... [v4,4/6] ENE 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: rc1 0x0, rc2 0x0; (no newline here) hwversion 0xc3, ediid 0x04 Change-Id: Id8e62bc9f6785b4bf0be0aaf0f74c8120d77c0d4 Signed-off-by: Mike Banon <mikebdp2(a)gmail.com> --- M edi.c 1 file changed, 15 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/61/23261/2 -- 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: 2 Gerrit-Owner: Mike Banon <mikebdp2(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>

1 0

Change in flashrom[master]: [v4, 6/6] Add support for reading the current flash contents from a file
by Mike Banon (Code Review) Jan. 31, 2018

Jan. 31, 2018

Mike Banon has posted comments on this change. ( https://review.coreboot.org/23263 ) Change subject: [v4,6/6] Add support for reading the current flash contents from a file ...................................................................... Patch Set 1: > Reading is faster in many cases now, is this still needed / wanted? Despite the small memory size (just 128 KB), all the operations with KB9012 internal flash through EDI interface are much much slower than for the regular flash chips. Sadly it takes 1 hour for the complete read/erase/write/read(verify) cycle. This patch tries to migitate this problem, it is essential for the comfortable development of KB9012 free-as-in-freedom firmware replacement -- 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: 1 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 Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 30 Jan 2018 23:01:42 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: [v4, 3/6] ENE EDI - add dummy read to ensure proper detection of ENE c...
by Mike Banon (Code Review) Jan. 30, 2018

Jan. 30, 2018

Mike Banon has posted comments on this change. ( https://review.coreboot.org/23260 ) Change subject: [v4,3/6] ENE EDI - add dummy read to ensure proper detection of ENE chips ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/#/c/23260/1/edi.c File edi.c: https://review.coreboot.org/#/c/23260/1/edi.c@500 PS1, Line 500: edi_detect(flash); > `edi_detect` sounds a little like it would already probe for it... […] Yes, edi_detect is a function that includes probing - but this function would never be called if KB9012 has not been explicitly selected by the user. One of the following patches moves KB9012 into the "hidden chips" domain - these chips would still shown as supported by flashrom as well as other chips, but a hidden chip (KB9012 in this case) isn't probed for by flashrom unless the user chooses it with -c option -- To view, visit https://review.coreboot.org/23260 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: I69ee71674649cd8ba4fc635f889cb39a1cd204b9 Gerrit-Change-Number: 23260 Gerrit-PatchSet: 1 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 Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 30 Jan 2018 22:45:46 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: [v4, 2/6] ENE Embedded Debug Interface EDI and ENE KB9012 EC flashing ...
by Mike Banon (Code Review) Jan. 30, 2018

Jan. 30, 2018

Mike Banon has posted comments on this change. ( https://review.coreboot.org/23259 ) Change subject: [v4,2/6] ENE Embedded Debug Interface EDI and ENE KB9012 EC flashing support ...................................................................... Patch Set 2: > Patch Set 2: Commit message was updated. This small change has caused a rebase (gerrit shows two patch sets although they are identical) -- 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: 2 Gerrit-Owner: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 30 Jan 2018 22:40:45 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: [v4, 1/6] Add support for selecting the erased bit value with a flag
by Mike Banon (Code Review) Jan. 30, 2018

Jan. 30, 2018

Mike Banon has posted comments on this change. ( https://review.coreboot.org/23258 ) Change subject: [v4,1/6] Add support for selecting the erased bit value with a flag ...................................................................... Patch Set 2: (5 comments) > (5 comments) > > Code looks good, how well was the rebase tested? > (5 comments) > > Code looks good, how well was the rebase tested? After I've tested the code and submitted these ported patches, the only rebase was because of a small typo fix at one of the commit messages, so the results are still positive - fully tested on the real hardware https://review.coreboot.org/#/c/23258/1//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/23258/1//COMMIT_MSG@2 PS1, Line 2: Author: Mike Banon <mikebdp2(a)gmail.com> > As a courtesy, you should reset the author to Paul, or use […] All my commit messages which contain Paul's code - are giving the credit to Paul by including the note "Original patch has been created by Paul Kocialkowski" with a link to it. I'm not the author of the original patches, but while porting these patches I've modified them to make them compatible with the latest flashrom. And still haven't received any comment from Paul - it would be wrong to put his signature when we still do not even know his opinion about these changed patches. Also, these modified patches could contain mistakes (although I tried not to make any) - in which case it is me the committer who will have to be blamed https://review.coreboot.org/#/c/23258/1//COMMIT_MSG@7 PS1, Line 7: [v4,1/6] > Please remove this tag. After these tags are removed, would it still be convenient to merge these patches in the correct order? (1/6->2/6->3/6->4/6->5/6->6/6) I'm just trying to follow Paul's scheme from patchwork, it seemed to me as a right approach https://review.coreboot.org/#/c/23258/1//COMMIT_MSG@17 PS1, Line 17: Signed-off-by: Mike Banon <mikebdp2(a)gmail.com> > Please also keep original Signed-off-by lines. These patches have been modified to make them compatible with the latest flashrom, not taken as-is. Not sure if the original signed-off-by still applies, because - since I've modified these patches - I should take the full responsibility for their quality https://review.coreboot.org/#/c/23258/1/flashrom.c File flashrom.c: https://review.coreboot.org/#/c/23258/1/flashrom.c@762 PS1, Line 762: static int need_erase_gran_bytes(const uint8_t *have, const uint8_t *want, unsigned int len, > line is too long (112 chars limit) fixed at new edit https://review.coreboot.org/#/c/23258/1/flashrom.c@792 PS1, Line 792: */ > line is too long fixed at new edit -- To view, visit https://review.coreboot.org/23258 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: Ia7b0de8568e31f9bf263ba0ad6b051e837477b6b Gerrit-Change-Number: 23258 Gerrit-PatchSet: 2 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 Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 30 Jan 2018 22:38:41 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: [v4, 1/6] Add support for selecting the erased bit value with a flag
by build bot (Jenkins) (Code Review) Jan. 30, 2018

Jan. 30, 2018

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23258 ) Change subject: [v4,1/6] Add support for selecting the erased bit value with a flag ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/922/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/1082/ : SUCCESS -- To view, visit https://review.coreboot.org/23258 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: Ia7b0de8568e31f9bf263ba0ad6b051e837477b6b Gerrit-Change-Number: 23258 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Banon <mikebdp2(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, 30 Jan 2018 22:35:51 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: [v4, 1/6] Add support for selecting the erased bit value with a flag
by Mike Banon (Code Review) Jan. 30, 2018

Jan. 30, 2018

Hello Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/23258 to look at the new patch set (#2). Change subject: [v4,1/6] Add support for selecting the erased bit value with a flag ...................................................................... [v4,1/6] Add support for selecting the erased bit value with a flag Most flash chips are erased to ones and programmed to zeros. However, some other chips, such as the ENE KB9012 internal flash, work the opposite way. Original patch has been created by Paul Kocialkowski, the previous version: http://patchwork.coreboot.org/patch/4412/ Now it has been ported to the latest source code of flashrom master branch Change-Id: Ia7b0de8568e31f9bf263ba0ad6b051e837477b6b Signed-off-by: Mike Banon <mikebdp2(a)gmail.com> --- M flash.h M flashrom.c 2 files changed, 26 insertions(+), 15 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/58/23258/2 -- To view, visit https://review.coreboot.org/23258 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: Ia7b0de8568e31f9bf263ba0ad6b051e837477b6b Gerrit-Change-Number: 23258 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Banon <mikebdp2(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>

1 0