Nico Huber 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.
I see. The patch generally looks good. Two things, though:
1. `contents file` sounds ambiguous, how about `reference file`?
2. The manpage needs an update. It should also be noted when the
data from the reference file is used (currently only at the begin-
ning, not later, e.g. when flash content is reread after a failed
erase).
> This patch tries
> to migitate this problem, it is essential for the comfortable
> development of KB9012 free-as-in-freedom firmware replacement
Does "free-as-in-freedom" mean anything to you, or are you only
trying to emphasize that this is important?
--
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: Fri, 02 Feb 2018 22:29:45 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No
Nico Huber 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: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/23261/2//COMMIT_MSG
Commit Message:
https://review.coreboot.org/#/c/23261/2//COMMIT_MSG@13
PS2, Line 13: (no newline here)
You can leave a long line in a commit message if it's a quote.
(If it doesn't fit here, it probably also doesn't fit in a terminal.)
--
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: Fri, 02 Feb 2018 22:14:58 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Nico Huber 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);
> Yes, edi_detect is a function that includes probing
I don't see that. I only see a dummy read.
--
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: Fri, 02 Feb 2018 22:08:10 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Nico Huber 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:
(3 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
I meant the original rebase of Paul's work. What is "the real hard-
ware"? It should be primarily tested on previously supported pro-
grammers and flash chips to make sure nothing regresses.
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>
> All my commit messages which contain Paul's code - are giving the credit to Paul by including the no […]
Well, that's why there are two fields "Author" and "Committer". If you
insist, I'll have to find out or make up what is allowed for this
project.
https://review.coreboot.org/#/c/23258/1//COMMIT_MSG@7
PS1, Line 7: [v4,1/6]
> After these tags are removed, would it still be convenient to merge these patches in the correct ord […]
The order is clear in Git. These tags are usually used in emails
but not in a repository.
(The order is wrong anyway, we need the option to exclude EDI from
probing first.)
https://review.coreboot.org/#/c/23258/1//COMMIT_MSG@17
PS1, Line 17: Signed-off-by: Mike Banon <mikebdp2(a)gmail.com>
> These patches have been modified to make them compatible with the latest flashrom, not taken as-is. […]
Ok that's fine. You are always free to take full responsibility.
--
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: Fri, 02 Feb 2018 22:04:38 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No