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 4:
> I happen to have a Lewisburg reference board handy, here is the
> output with PS4: https://paste.flashrom.org/view.php?id=3046
Cool. This looks odd:
> Descr. BIOS ME GbE Platf.
> BIOS w w r
I'll have closer look at the 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: 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>
Gerrit-Comment-Date: Mon, 21 Aug 2017 20:33:23 +0000
Gerrit-HasComments: No
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 4:
(4 comments)
The list in layout_from_ich_descriptors() should be updated too.
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c
File ich_descriptors.c:
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@320
PS4, Line 320: unknown
Region 6 (counting from 0) is the Secondary BIOS region, according to
some register's descriptions that control it.
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@321
PS4, Line 321: EC
IIRC, this is "BMC" for Lewisburg. Maybe name it "EC/BMC"?
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@1133
PS4, Line 1133: };
I just remembered, here's a third list. Though, this one uses identi-
fiers compatible to the ones in ifdtool from coreboot.
The -N and --ifd options could also be helpful to make flashrom work
with unallocated space in the chip, like the gaps Sandy has (as far
as I followed the discussion).
https://review.coreboot.org/#/c/20922/4/util/ich_descriptors_tool/ich_descr…
File util/ich_descriptors_tool/ich_descriptors_tool.c:
https://review.coreboot.org/#/c/20922/4/util/ich_descriptors_tool/ich_descr…
PS4, Line 48: };
That's a lot of maintenance, maybe we should move the list from
pprint_freg() into the header file and use it instead?
--
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: 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>
Gerrit-Comment-Date: Mon, 21 Aug 2017 20:24: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 4:
I forgot to add the region descriptions in ichspi.c, so PS4 adds them. Also, "GbE" is used for consistency (instead of "GBE").
I happen to have a Lewisburg reference board handy, here is the output with PS4: https://paste.flashrom.org/view.php?id=3046
--
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: 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>
Gerrit-Comment-Date: Mon, 21 Aug 2017 20:10:35 +0000
Gerrit-HasComments: No
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 4:
(2 comments)
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c
File ich_descriptors.c:
https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@321
PS4, Line 321: "IE", "10GbE", "unknown", "unknown", "unknown", "unknown"
Added these in PS4
https://review.coreboot.org/#/c/20922/4/util/ich_descriptors_tool/ich_descr…
File util/ich_descriptors_tool/ich_descriptors_tool.c:
https://review.coreboot.org/#/c/20922/4/util/ich_descriptors_tool/ich_descr…
PS4, Line 46: GbE
Changed this from "GBE" to "GbE" to be consistent with the other region with "GbE" in the name.
--
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: 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>
Gerrit-Comment-Date: Mon, 21 Aug 2017 20:09:16 +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 (#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