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 (#4).
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, 81 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/20922/4
--
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: 4
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 3: Code-Review+2
(2 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) {
> Done.
Agreed.
Another option would be to enforce an order in the enum and check for
>= CHIPSET_100_SERIES_SUNRISE_POINT. That might work for some time but
it depends on future incompatibilities, of course, when we'll finally
have to use a `switch` anyway.
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 */
> 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?
Sorry, that was confusing. I tested this on Sunrise Point not
Lewisburg (don't have the latter). The Sunrise Point datasheet
says 6 while 10 registers are implement. I expect Lewisburg to
implement 12 like the datasheet suggests.
>
> LMK your thoughts on how to proceed from here.
Current verison looks good. Do you want to trigger Sandy to
test it?
--
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 11:51:11 +0000
Gerrit-HasComments: Yes
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>