David Hendricks has posted comments on this change. ( https://review.coreboot.org/20922 )
Change subject: chispet_enable: Add support for C620-series Lewisburg PCH
......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/#/c/20922/2/chipset_enable.c
File chipset_enable.c:
https://review.coreboot.org/#/c/20922/2/chipset_enable.c@836
PS2, Line 836: or_
> s/and/or/? (ich you read the identifier as a statement...)
Done
https://review.coreboot.org/#/c/20922/2/ich_descriptors.c
File ich_descriptors.c:
https://review.coreboot.org/#/c/20922/2/ich_descriptors.c@76
PS2, Line 76: default:
: if (cont->N
> You could remove both else paths and just return -1 at the end.
Done.
https://review.coreboot.org/#/c/20922/2/ichspi.c
File ichspi.c:
https://review.coreboot.org/#/c/20922/2/ichspi.c@398
PS2, Line 398: ich_generation == CHIPSET_C620_SERIES_LEWISBURG) {
> Please don't align the indendation with the body.
Done.
BTW - I wonder if we should change most of these `if (ich_generation == ...)` statements to `switch (ich_generation)`. That would solve any indentation issue now and moving forward, and would also be more uniform with other parts of the ichspi and ich_desctriptors code that have to deal with multiple generations.
https://review.coreboot.org/#/c/20922/2/ichspi.c@1703
PS2, Line 1703: num_freg = 12; /* 12 MMIO regs, but 16 regions in FD spec */
> 10 is correct, the registers read with 0x00007fff just what is set
So should we leave it at 12 to match the datasheet, even though FREG10 and 11 read 0 and are ignored? Should we explain this behavior in the comment?
LMK your thoughts on how to proceed from here.
https://review.coreboot.org/#/c/20922/2/ichspi.c@1704
PS2, Line 1704: 6; /* Includes GPR0 */
> 6 including the GPR0 register, sorry should have added a comment...
Ah, that makes sense. I went ahead and added a comment for Sunrise Point and Lewisburg. ICH8/9 don't have a GPR register, so 5 is already correct to account for PR0-PR4.
--
To view, visit https://review.coreboot.org/20922
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: comment
Gerrit-Change-Id: I96c89bc28bdfcd953229c17679f2c28f8b874d0b
Gerrit-Change-Number: 20922
Gerrit-PatchSet: 3
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(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: Mon, 21 Aug 2017 04:41:32 +0000
Gerrit-HasComments: Yes
Hello Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/20922
to look at the new patch set (#3).
Change subject: chispet_enable: Add support for C620-series Lewisburg PCH
......................................................................
chispet_enable: Add support for C620-series Lewisburg PCH
This adds PCI IDs for C620-series PCHs and adds
CHIPSET_C620_SERIES_LEWISBURG as a new entry in the ich_chipset enum.
Lewisburg is very similar to Sunrise Point for Flashrom's purposes,
however one important difference is the way the "number of masters" is
interpreted from the flash descriptor (0-based vs. 1-based).
Change-Id: I96c89bc28bdfcd953229c17679f2c28f8b874d0b
Signed-off-by: David Hendricks <dhendricks(a)fb.com>
---
M chipset_enable.c
M ich_descriptors.c
M ichspi.c
M programmer.h
M util/ich_descriptors_tool/ich_descriptors_tool.c
5 files changed, 80 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/20922/3
--
To view, visit https://review.coreboot.org/20922
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I96c89bc28bdfcd953229c17679f2c28f8b874d0b
Gerrit-Change-Number: 20922
Gerrit-PatchSet: 3
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(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>
Nico Huber has posted comments on this change. ( https://review.coreboot.org/20922 )
Change subject: chispet_enable: Add support for C620-series Lewisburg PCH
......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/20922/2/chipset_enable.c
File chipset_enable.c:
https://review.coreboot.org/#/c/20922/2/chipset_enable.c@836
PS2, Line 836: and
s/and/or/? (ich you read the identifier as a statement...)
https://review.coreboot.org/#/c/20922/2/ich_descriptors.c
File ich_descriptors.c:
https://review.coreboot.org/#/c/20922/2/ich_descriptors.c@76
PS2, Line 76: else
: return -1;
You could remove both else paths and just return -1 at the end.
https://review.coreboot.org/#/c/20922/2/ichspi.c
File ichspi.c:
https://review.coreboot.org/#/c/20922/2/ichspi.c@1703
PS2, Line 1703: num_freg = 12; /* 12 MMIO regs, but 16 regions in FD spec */
> Depending on what the regular PCH returns for the register addresses,
10 is correct, the registers read with 0x00007fff just what is set
in the descriptor.
12 would work too for the regular PCH (registers read 0 and are
ignored by the current code).
--
To view, visit https://review.coreboot.org/20922
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: comment
Gerrit-Change-Id: I96c89bc28bdfcd953229c17679f2c28f8b874d0b
Gerrit-Change-Number: 20922
Gerrit-PatchSet: 2
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(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: Sat, 19 Aug 2017 13:45:19 +0000
Gerrit-HasComments: Yes
Nico Huber has posted comments on this change. ( https://review.coreboot.org/20922 )
Change subject: chispet_enable: Add support for C620-series Lewisburg PCH
......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/#/c/20922/2/ichspi.c
File ichspi.c:
https://review.coreboot.org/#/c/20922/2/ichspi.c@398
PS2, Line 398: ich_generation == CHIPSET_C620_SERIES_LEWISBURG) {
Please don't align the indendation with the body.
https://review.coreboot.org/#/c/20922/2/ichspi.c@1557
PS2, Line 1557: (((ICH_BRRA(frap) >> i) & 1) << 0);
Overflow for i >= 8 (have a fix in progress somewhere).
https://review.coreboot.org/#/c/20922/2/ichspi.c@1600
PS2, Line 1600: /* From 5 on we have GPR registers and start from 0 again. */
Oh, here's the comment ;)
https://review.coreboot.org/#/c/20922/2/ichspi.c@1703
PS2, Line 1703: num_freg = 12; /* 12 MMIO regs, but 16 regions in FD spec */
Depending on what the regular PCH returns for the register addresses,
12 might not hurt there either. I'm not sure any more why I set it
to 10, maybe based on some observation rather than the datasheet...
in the end, we only dump the registers anyway.
https://review.coreboot.org/#/c/20922/2/ichspi.c@1704
PS2, Line 1704: 6; /* FIXME: should be 5? */
> num_pr = 6 seemed wrong for both Sunrise Point and Lewisburg, but maybe I a
6 including the GPR0 register, sorry should have added a comment...
--
To view, visit https://review.coreboot.org/20922
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: comment
Gerrit-Change-Id: I96c89bc28bdfcd953229c17679f2c28f8b874d0b
Gerrit-Change-Number: 20922
Gerrit-PatchSet: 2
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(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: Sat, 19 Aug 2017 13:03:20 +0000
Gerrit-HasComments: Yes
David Hendricks has posted comments on this change. ( https://review.coreboot.org/20937 )
Change subject: ich_descriptors: Fix off-by-one error
......................................................................
Patch Set 1: Code-Review-2
Out of caution, I went ahead and added Lewisburg as its own ICH in https://review.coreboot.org/#/c/20922/ . So far the only notable differences I've come across are the number of masters and maybe the number of protected range registers. Let's discuss further in 20922.
--
To view, visit https://review.coreboot.org/20937
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: comment
Gerrit-Change-Id: I92627332265cf79720b98241d73bc36b1f6a7167
Gerrit-Change-Number: 20937
Gerrit-PatchSet: 1
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Sat, 19 Aug 2017 03:18:13 +0000
Gerrit-HasComments: No