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/+/60272 )
Change subject: ichspi: Remove unneeded line breaks, add useful ones
......................................................................
Patch Set 2:
(6 comments)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/60272/comment/5b093da4_69c140e6
PS1, Line 930: REGREAD16(ICH7_REG_SPIS));
This too
https://review.coreboot.org/c/flashrom/+/60272/comment/46028a97_5e831f84
PS1, Line 999: SSFC_DBC);
I would remove this line break.
https://review.coreboot.org/c/flashrom/+/60272/comment/14cbee9a_de8cea28
PS1, Line 1052: REGREAD32(swseq_data.reg_ssfsc));
This too
https://review.coreboot.org/c/flashrom/+/60272/comment/3097ac95_b252a912
PS1, Line 1127: cmd);
This too
https://review.coreboot.org/c/flashrom/+/60272/comment/c991f56e_80a412c1
PS1, Line 1402: "N
This too
https://review.coreboot.org/c/flashrom/+/60272/comment/68270b8c_a8652b73
PS1, Line 1720:
> Looking at the rest of the file, there are more. So it seems to be intended, but still weird.
Ah! I think I understand this now. This is an extension so that the contents in these brackets are on the same character level. Well. Sorry for the noise :) Done.
--
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: 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 06:01:28 +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.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/58736
to look at the new patch set (#3).
Change subject: ichspi: Extract handling programmer param into a function
......................................................................
ichspi: Extract handling programmer param into a function
Extract processing of ich_spi_mode into a separate function which
is called from init_ich_default. This makes init_ich_default more
readable and avoids one local variable.
BUG=b:204488958
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)
Change-Id: I20e2379a6fd58c9346f0a2d6daf2b8decf1f6976
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M ichspi.c
1 file changed, 37 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/36/58736/3
--
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: 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-MessageType: newpatchset
Attention is currently required from: Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/58737
to look at the new patch set (#3).
Change subject: ichspi: Extract initialisation of swseq and hwseq into a function
......................................................................
ichspi: Extract initialisation of swseq and hwseq into a function
Initialisation of swseq_data and hwseq_data gets its own function,
which is called from init_ich_default. This makes init_ich_default
more readable.
This patch also gives a name to (previously anonymous) struct
swseq_data. Its sibling struct hwseq_data already has a name. Structs
need names to be able to declare function parameters.
BUG=b:204488958
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)
Change-Id: I7d62b1b380e497b82dcae1284d752204cc541bd3
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M ichspi.c
1 file changed, 57 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/37/58737/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/58737
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7d62b1b380e497b82dcae1284d752204cc541bd3
Gerrit-Change-Number: 58737
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/58735
to look at the new patch set (#3).
Change subject: ichspi: Split very long init function into two
......................................................................
ichspi: Split very long init function into two
ich_init_spi is very long, but logically it can be split. Init
function detects the chipset and then the rest of operations depends
on the chipset.
Init function is more readable now, it consists of only a switch.
Initialisation of hwseq and swseq that used to happen in the
beginning of init function now moved to init_ich_default, because
hwseq and swseq are only used for chipsets served by init_ich_default.
BUG=b:204488958
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)
Change-Id: I6789bc456a4878e6555831ae0b80ecbdbf62938b
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M ichspi.c
1 file changed, 268 insertions(+), 255 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/35/58735/3
--
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan, Angel Pons, Nikolai Artemiev.
Anastasia Klimchuk 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 2:
(7 comments)
Patchset:
PS1:
> ICH7 does not support hardware sequencing (hwseq).
To close the comment: I could not find devices with older chipsets, but Nico saved me and tested those!
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/58735/comment/e27aec77_250307e6
PS1, Line 1717: msg_pdbg("0x00: 0x%04x (SPIS)\n",
: mmio_readw(spibar + 0));
: msg_pdbg("0x02: 0x%04x (SPIC)\n",
: mmio_readw(spibar + 2));
: msg_pdbg("0x04: 0x%08x (SPIA)\n",
: mmio_readl(spibar + 4));
: ichspi_bbar = mmio_readl(spibar + 0x50);
: msg_pdbg("0x50: 0x%08x (BBAR)\n",
: ichspi_bbar);
: msg_pdbg("0x54: 0x%04x (PREOP)\n",
: mmio_readw(spibar + 0x54));
: msg_pdbg("0x56: 0x%04x (OPTYPE)\n",
: mmio_readw(spibar + 0x56));
: msg_pdbg("0x58: 0x%08x (OPMENU)\n",
: mmio_readl(spibar + 0x58));
: msg_pdbg("0x5c: 0x%08x (OPMENU+4)\n",
: mmio_readl(spibar + 0x5c));
> Please remove unnecessary line breaks. If you put tabs after the commas, […]
Done in CB:60272. I actually discovered there is one line of code hidden between lots of debug messages! So I added new lines around it.
https://review.coreboot.org/c/flashrom/+/58735/comment/c065bebe_eb7d2c88
PS1, Line 1739: mmio_readl(spibar + offs), i);
> I guess this also doesn't need a line break.
Done in CB:60272
https://review.coreboot.org/c/flashrom/+/58735/comment/2454aea2_a4687793
PS1, Line 1836: arg);
> This is addressed in the follow-up commit, can we leave this as-is in this commit?
Oh yes, I removed this line break in CB:58736 , probably because I read a comment in that other patch first :) Is it alright?
https://review.coreboot.org/c/flashrom/+/58735/comment/4da5ee5e_73a11c84
PS1, Line 1917: );
> yeah, the line break before `);` looks really odd.
Done in CB:60272. So many line breaks that I decided all of them can go into a separate commit.
https://review.coreboot.org/c/flashrom/+/58735/comment/56b79b0c_a7fe0eb3
PS1, Line 2049: else {
: register_spi_master(&spi_master_ich9, NULL);
: }
> > Not in this commit, please. […]
Do you mind if I do this idea separately (not in this chain)? I don't want to grow this chain too long.
There is also another idea in another patch, and I know for sure I will be doing more work on ichspi next year.
https://review.coreboot.org/c/flashrom/+/58735/comment/b40d821e_a3774449
PS1, Line 2068: return 0;
> You've replaced the `break` statements above with `return` ones. So this […]
Compiler says fine!
--
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: 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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 22 Dec 2021 05:54:20 +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: 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/+/60272 )
Change subject: ichspi: Remove unneeded line breaks, add useful ones
......................................................................
Patch Set 1:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/60272/comment/fbc92123_7322f284
PS1, Line 1720:
> This looks weird. Are these spaces intended? Otherwise I would remove them.
Looking at the rest of the file, there are more. So it seems to be intended, but still weird.
--
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: 1
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 05:39:19 +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: Nico Huber, 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:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/60113/comment/fcec748d_15ae10e8
PS4, Line 9: Line up physmap with the other hwaccess related code.
> Oh, we could make it a hwaccess/ subdir of src/. No wait, there is […]
My plan is to move this to (src)/platform/...
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/60113/comment/61177e8d_c989c56e
PS4, Line 41: #if CONFIG_INTERNAL == 1
> Why the guard and why only here? It's just function prototypes, they […]
This isn't needed. Just a bad habit I got from flashrom
--
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: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Tue, 21 Dec 2021 20:38:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons.
Thomas Heijligen 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:
(1 comment)
File Makefile:
https://review.coreboot.org/c/flashrom/+/60111/comment/dfcfbdc5_34d696cd
PS4, Line 786: x86
> Same here, if it has x86 in the name, guard it?
Done
--
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: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Tue, 21 Dec 2021 20:32:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment