August 2017 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by Nico Huber (Code Review) Aug. 21, 2017

Aug. 21, 2017

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: (5 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@371 PS4, Line 371: if (cs == CHIPSET_100_SERIES_SUNRISE_POINT) { missing C620 here https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@373 PS4, Line 373: EC EC/BMC? https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@373 PS4, Line 373: unknown 4th region is Device Expansion, DE? https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@373 PS4, Line 373: , We have to add a sixth region, "unknown", or we'd run into the check below. https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@381 PS4, Line 381: msg_pdbg2(" FD BIOS ME GbE Pltf Reg5 Reg6 Reg7 EC Reg9\n"); More regions, sigh. -- 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:44:39 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by Nico Huber (Code Review) Aug. 21, 2017

Aug. 21, 2017

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

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by Nico Huber (Code Review) Aug. 21, 2017

Aug. 21, 2017

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

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by David Hendricks (Code Review) Aug. 21, 2017

Aug. 21, 2017

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

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by David Hendricks (Code Review) Aug. 21, 2017

Aug. 21, 2017

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

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by build bot (Jenkins) (Code Review) Aug. 21, 2017

Aug. 21, 2017

build bot (Jenkins) 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: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/520/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/477/ : SUCCESS -- 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:07:01 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by David Hendricks (Code Review) Aug. 21, 2017

Aug. 21, 2017

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>

1 0

Change in flashrom[staging]: chispet_enable: Add support for C620-series Lewisburg PCH
by Nico Huber (Code Review) Aug. 21, 2017

Aug. 21, 2017

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

1 0

Change in flashrom[staging]: ich_descriptors: Fix off-by-one error
by David Hendricks (Code Review) Aug. 21, 2017

Aug. 21, 2017

David Hendricks has abandoned this change. ( https://review.coreboot.org/20937 ) Change subject: ich_descriptors: Fix off-by-one error ...................................................................... Abandoned -- To view, visit https://review.coreboot.org/20937 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: abandon 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>

1 0

Change in flashrom[staging]: ichspi: "Fix" access permission reporting for regions > 7
by David Hendricks (Code Review) Aug. 21, 2017

Aug. 21, 2017

David Hendricks has posted comments on this change. ( https://review.coreboot.org/21106 ) Change subject: ichspi: "Fix" access permission reporting for regions > 7 ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/21106 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ib73f95e0348f5c6d89988e3ea3529af0ec3b23a6 Gerrit-Change-Number: 21106 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> 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:44:08 +0000 Gerrit-HasComments: No

1 0