Attention is currently required from: Angel Pons.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60113 )
Change subject: physmap: rename to hwaccess_physmap, create own header
......................................................................
Patch Set 5:
(1 comment)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/60113/comment/a8e47254_e1331b11
PS4, Line 41: #if CONFIG_INTERNAL == 1
> Is it contagious? :#
Jop, to many #if and friends around here. ;)
--
To view, visit https://review.coreboot.org/c/flashrom/+/60113
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ieba6f4e94cfc3e668fcb8b3c978de5908aed2592
Gerrit-Change-Number: 60113
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 22 Dec 2021 12:23:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60111 )
Change subject: hwaccess physmap: move x86 msr related code into own files
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/60111
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idc9ce9df3ea1e291ad469de59467646b294119c4
Gerrit-Change-Number: 60111
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 22 Dec 2021 12:18:56 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60110 )
Change subject: hwaccess: move x86 port I/O related code into own files
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
File Makefile:
https://review.coreboot.org/c/flashrom/+/60110/comment/c01c5b9e_04616157
PS5, Line 786: x86
> Done
Thanks!
And good idea to shuffle the if's btw. :)
--
To view, visit https://review.coreboot.org/c/flashrom/+/60110
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a
Gerrit-Change-Number: 60110
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 22 Dec 2021 12:17:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60272 )
Change subject: ichspi: Remove unneeded line breaks, add useful ones
......................................................................
Patch Set 2:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/60272/comment/4e76dba8_c67e5ec4
PS1, Line 1720:
> Ah! I think I understand this now. […]
Yes, this is visual alignment of the printed messages
--
To view, visit https://review.coreboot.org/c/flashrom/+/60272
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7ca2902b7caaa95418b828b068c661afafdcd171
Gerrit-Change-Number: 60272
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 22 Dec 2021 10:39:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58735 )
Change subject: ichspi: Split very long init function into two
......................................................................
Patch Set 3: Code-Review+1
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/58735/comment/714ccfec_6abd0111
PS3, Line 20: TEST=1) probe-read-verify-erase section-write-reboot
: on Intel octopus board with GD25LQ128C/GD25LQ128D/GD25LQ128E
: 2) probe and read on Panther Point (7 series PCH)
> Personally, I don't really like these "tags" (here TEST or in other commits sometimes TESTED), becau […]
If you still need a `TEST=` tag for some reason, you could do something like this:
TEST=Check that the following scenarios still behave properly:
1) Probe-read-verify-erase section-write-reboot
on Intel octopus board with GD25LQ128C/GD25LQ128D/GD25LQ128E
2) Probe and read on Panther Point (7 series PCH)
Note that the text following `TEST=` provides information about how the test results were interpreted. Without this part, it might be that both tests failed spectacularly, but the commit message wouldn't be lying: the change *was* tested, but without success! 😄
And yes, manually adding Tested-by tags is perfectly fine. I only use them when someone else has tested my changes. I feel a Tested-by tag is redundant when I've tested my own changes.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/58735/comment/10efe72b_6a8e8ae0
PS2, Line 1822: arg = extract_programmer_param("ich_spi_mode");
> Wow! this is great idea, I might use it in future, thank you!!
> I would leave this commit as is, hope it's fine.
Sure, no problem!
https://review.coreboot.org/c/flashrom/+/58735/comment/f33a90f0_5a50853e
PS2, Line 1769:
> Yes this was accidental, I removed it.
Thanks!
--
To view, visit https://review.coreboot.org/c/flashrom/+/58735
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6789bc456a4878e6555831ae0b80ecbdbf62938b
Gerrit-Change-Number: 58735
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 22 Dec 2021 10:27:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58736 )
Change subject: ichspi: Extract handling programmer param into a function
......................................................................
Patch Set 3:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/58736/comment/db443982_9209e503
PS1, Line 1761: if (arg && !strcmp(arg, "hwseq")) {
> Nice to see you here Felix :) All good that was not blocking. […]
Well, it's about reworking (rather rewriting) the arguments parser. I don't think people will like this since this will cause a lot of review work 😄
I will look at the current state and see how it is currently done. Maybe it's not even possible.
--
To view, visit https://review.coreboot.org/c/flashrom/+/58736
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I20e2379a6fd58c9346f0a2d6daf2b8decf1f6976
Gerrit-Change-Number: 58736
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 22 Dec 2021 07:38:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58736 )
Change subject: ichspi: Extract handling programmer param into a function
......................................................................
Patch Set 3:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/58736/comment/eeb7369f_06396c50
PS2, Line 1764: arg
> Just checking: this idea can go in a separate commit, right?
I think so. I would do that in a separate commit so that changes don't get mixed up.
--
To view, visit https://review.coreboot.org/c/flashrom/+/58736
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I20e2379a6fd58c9346f0a2d6daf2b8decf1f6976
Gerrit-Change-Number: 58736
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 22 Dec 2021 07:26:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58735 )
Change subject: ichspi: Split very long init function into two
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/58735/comment/3c599105_133c98b5
PS3, Line 20: TEST=1) probe-read-verify-erase section-write-reboot
: on Intel octopus board with GD25LQ128C/GD25LQ128D/GD25LQ128E
: 2) probe and read on Panther Point (7 series PCH)
> Did I do the right by adding test info into commit message? Do I need to added Tested-By tags? I don […]
Personally, I don't really like these "tags" (here TEST or in other commits sometimes TESTED), because this way you are not able to write multiple lines which still fit to the style of the rest of the commit message. So, what people do is, they write half broken sentences (to shorten the characters count) and sometimes I have no idea what they are trying to tell me or what the meaning is.
However, I would only use tags if the text behind it fits in the same line. Otherwise, like in your case, I would write it as follow and I would move it before that BUG tag, to make it part of the "actual" commit summary (since I think it is relevant information):
Tests done:
1) Probe-read-verify-erase section-write-reboot
on Intel octopus board with GD25LQ128C/GD25LQ128D/GD25LQ128E
2) Probe and read on Panther Point (7 series PCH)
Of course you can add a Tested-by tag for another person. Even if it is not you, it adds useful information for the readers, since they are able to contact these persons then. There might be cases, where there are multiple ways to test things. Everyone might use a different way and each one might give another experience (not only in knowledge, but also in usability and meaningfulness). So this is definitely useful :)
--
To view, visit https://review.coreboot.org/c/flashrom/+/58735
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6789bc456a4878e6555831ae0b80ecbdbf62938b
Gerrit-Change-Number: 58735
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 22 Dec 2021 07:07:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment