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

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

Aug. 30, 2017

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20922 ) Change subject: chipset_enable: Add support for C620-series Lewisburg PCH ...................................................................... Patch Set 5: Code-Review+2 -- 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: 5 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: Wed, 30 Aug 2017 14:08:10 +0000 Gerrit-HasComments: No

1 0

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

Aug. 29, 2017

David Hendricks has posted comments on this change. ( https://review.coreboot.org/20922 ) Change subject: chipset_enable: Add support for C620-series Lewisburg PCH ...................................................................... Patch Set 5: friendly ping -- 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: 5 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, 28 Aug 2017 23:07:04 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[refs/meta/config]: Refine rights to create/delete any branches
by build bot (Jenkins) (Code Review) Aug. 24, 2017

Aug. 24, 2017

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/21090 ) Change subject: Refine rights to create/delete any branches ...................................................................... Patch Set 2: Build Successful https://qa.coreboot.org/job/flashrom-customrules/523/ : SUCCESS -- To view, visit https://review.coreboot.org/21090 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: refs/meta/config Gerrit-MessageType: comment Gerrit-Change-Id: Ia8ad1dc3dcdb6e35bfb47cbe437f2f7ea1cc560b Gerrit-Change-Number: 21090 Gerrit-PatchSet: 2 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> 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: Thu, 24 Aug 2017 09:31:46 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[refs/meta/config]: Refine rights to create/delete any branches
by Patrick Georgi (Code Review) Aug. 24, 2017

Aug. 24, 2017

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/21090 ) Change subject: Refine rights to create/delete any branches ...................................................................... Refine rights to create/delete any branches Allow creation/deletion of any branches (instead of just stable/staging) and give these rights to David, Nico and myself for now. Change-Id: Ia8ad1dc3dcdb6e35bfb47cbe437f2f7ea1cc560b Reviewed-on: https://review.coreboot.org/21090 Tested-by: Stefan Tauner <stefan.tauner(a)gmx.at> Reviewed-by: Patrick Georgi <pgeorgi(a)google.com> --- M groups M project.config 2 files changed, 9 insertions(+), 5 deletions(-) Approvals: Stefan Tauner: Verified Patrick Georgi: Looks good to me, approved diff --git a/groups b/groups index 3caeb32..5cb19fc 100644 --- a/groups +++ b/groups @@ -1,5 +1,6 @@ # UUID Group Name # db95ce11b379445ac8c5806ea0b61195555b338d flashrom developers +user:Nico user/Nico Huber (Nico) user:dhendrix user/David Hendricks (dhendrix) user:stefanct user/Stefan Tauner (stefanct) diff --git a/project.config b/project.config index e49aeed..a05ec32 100644 --- a/project.config +++ b/project.config @@ -31,11 +31,14 @@ header = to filter = -is:draft [access "refs/heads/master"] - create = group user/David Hendricks (dhendrix) - create = group user/Stefan Tauner (stefanct) - delete = group user/David Hendricks (dhendrix) - delete = group user/Stefan Tauner (stefanct) - exclusiveGroupPermissions = create delete [access "refs/*"] label-Verified = -1..+1 group user/Stefan Tauner (stefanct) submit = group user/Stefan Tauner (stefanct) +[access "refs/heads/*"] + exclusiveGroupPermissions = create delete + create = group user/David Hendricks (dhendrix) + create = group user/Nico Huber (Nico) + create = group user/Stefan Tauner (stefanct) + delete = group user/David Hendricks (dhendrix) + delete = group user/Nico Huber (Nico) + delete = group user/Stefan Tauner (stefanct) -- To view, visit https://review.coreboot.org/21090 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: refs/meta/config Gerrit-MessageType: merged Gerrit-Change-Id: Ia8ad1dc3dcdb6e35bfb47cbe437f2f7ea1cc560b Gerrit-Change-Number: 21090 Gerrit-PatchSet: 2 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> 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]: chipset_enable: Add support for C620-series Lewisburg PCH
by build bot (Jenkins) (Code Review) Aug. 22, 2017

Aug. 22, 2017

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/20922 ) Change subject: chipset_enable: Add support for C620-series Lewisburg PCH ...................................................................... Patch Set 5: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/522/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/478/ : 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: 5 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: Tue, 22 Aug 2017 03:27:18 +0000 Gerrit-HasComments: No

1 0

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

Aug. 22, 2017

David Hendricks has posted comments on this change. ( https://review.coreboot.org/20922 ) Change subject: chipset_enable: Add support for C620-series Lewisburg PCH ...................................................................... Patch Set 4: (9 comments) New results: https://paste.flashrom.org/view.php?id=3047 https://review.coreboot.org/#/c/20922/4//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/20922/4//COMMIT_MSG@7 PS4, Line 7: sp > chi*ps*et oops - done. https://review.coreboot.org/#/c/20922/4/ich_descriptors.c File ich_descriptors.c: https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@292 PS4, Line 292: desc->component.FLILL1 == 0) { > Just realized that you changed parentheses here. Which is wrong. Darn! It felt a bit awkward since I also had to invert logic and a add set of parens. I tried to make this a bit more idiot-proof by breaking apart the if-statements for FLILL/FLILL1 and the chipset. This way we only need to check the chipset once, and it will make it easier to use a switch statement later if we want. At the very least it should require fewer functioning brain cells when going thru the tedious effort of adding a new chipset. 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 I went ahead and named it BIOS2 as suggested (it was one of a few suggestions actually) on IRC. 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"? Done https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@371 PS4, Line 371: if (cs == CHIPSET_100_SERIES_SUNRISE_POINT) { > missing C620 here Done. I gave it its own special case since since we have a longer region name length (5 chars instead of 4). Here's how it looks (I'll post another example in case gerrit mangles this): --- Details --- FD BIOS ME GbE Pltf DE BIOS2 Reg7 BMC DE2 IE 10GbE OpROM Reg13 Reg14 Reg15 BIOS rw rw rw rw rw r ME r rw rw r GbE rw DE rw rw rw BMC rw IE rw 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 Yes, 4th region is DE. Since I'm not sharing the Sunrise Point code, I guess this can just be "BMC"? Though in the future some PCH that calls it "EC" will re-use the Lewisburg codepath, in which case we can change it. 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. Done 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- I went ahead and updated this list for what I would expect ifdtool to use, though I haven't gone thru all of ifdtool yet. I thought access to unallocated space was generally forbidden. I'm not certain of that, though. 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 Yes, in another patch :-) -- 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: Tue, 22 Aug 2017 03:26:47 +0000 Gerrit-HasComments: Yes

1 0

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

Aug. 22, 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 (#5). Change subject: chipset_enable: Add support for C620-series Lewisburg PCH ...................................................................... chipset_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, 119 insertions(+), 28 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/20922/5 -- 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: 5 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. 22, 2017

Aug. 22, 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: (1 comment) https://review.coreboot.org/#/c/20922/4/ich_descriptors.c File ich_descriptors.c: https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@292 PS4, Line 292: desc->component.FLILL1 == 0) { Just realized that you changed parentheses here. Which is wrong. Now, `FLILL1 == 0` is checked on older platforms too and suffices to say "No forbidden opcodes". Maybe it becomes clearer if we invert the condition (and switch bodies): if (desc->component.FLILL != 0 || ((cs == CHIPSET_100_SERIES_SUNRISE_POINT || cs == CHIPSET_C620_SERIES_LEWISBURG) && desc->component.FLILL1 != 0)) { ... } else { msg_pdbg2("No forbidden opcodes.\n"); } -- 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 22:01:26 +0000 Gerrit-HasComments: Yes

1 0

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

Aug. 21, 2017

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/21106 ) Change subject: ichspi: "Fix" access permission reporting for regions > 7 ...................................................................... ichspi: "Fix" access permission reporting for regions > 7 Can't find bits that tell us the actual permissions in charge. So report them as unknown. Change-Id: Ib73f95e0348f5c6d89988e3ea3529af0ec3b23a6 Signed-off-by: Nico Huber <nico.huber(a)secunet.com> Reviewed-on: https://review.coreboot.org/21106 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: David Hendricks <david.hendricks(a)gmail.com> --- M ichspi.c 1 file changed, 18 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified David Hendricks: Looks good to me, approved diff --git a/ichspi.c b/ichspi.c index 5f3e394..9ee8044 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1543,6 +1543,7 @@ static const char *const access_names[4] = { "locked", "read-only", "write-only", "read-write" }; + const int rwperms_unknown = ARRAY_SIZE(access_names); static const char *const region_names[5] = { "Flash Descriptor", "BIOS", "Management Engine", "Gigabit Ethernet", "Platform Data" @@ -1550,10 +1551,20 @@ const char *const region_name = i < ARRAY_SIZE(region_names) ? region_names[i] : "unknown"; uint32_t base, limit; - int rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) | - (((ICH_BRRA(frap) >> i) & 1) << 0); + int rwperms; int offset = ICH9_REG_FREG0 + i * 4; uint32_t freg = mmio_readl(ich_spibar + offset); + + if (i < 8) { + rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) | + (((ICH_BRRA(frap) >> i) & 1) << 0); + } else { + /* Datasheets don't define any access bits for regions > 7. We + can't rely on the actual descriptor settings either as there + are several overrides for them (those by other masters are + not even readable by us, *shrug*). */ + rwperms = rwperms_unknown; + } base = ICH_FREG_BASE(freg); limit = ICH_FREG_LIMIT(freg); @@ -1569,6 +1580,11 @@ region_name, base, limit, access_names[rwperms]); return 0; } + if (rwperms == rwperms_unknown) { + msg_pdbg("FREG%i: %s region (0x%08x-0x%08x) has unknown permissions.\n", + i, region_name, base, limit); + return 0; + } msg_pwarn("FREG%i: Warning: %s region (0x%08x-0x%08x) is %s.\n", i, region_name, base, limit, access_names[rwperms]); -- To view, visit https://review.coreboot.org/21106 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: merged Gerrit-Change-Id: Ib73f95e0348f5c6d89988e3ea3529af0ec3b23a6 Gerrit-Change-Number: 21106 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> 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 4: (2 comments) Grep'ed for 100_SERIES and spotted one more place where C620 is missing. https://review.coreboot.org/#/c/20922/4//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/20922/4//COMMIT_MSG@7 PS4, Line 7: sp chi*ps*et https://review.coreboot.org/#/c/20922/4/ich_descriptors.c File ich_descriptors.c: https://review.coreboot.org/#/c/20922/4/ich_descriptors.c@303 PS4, Line 303: if (cs == CHIPSET_100_SERIES_SUNRISE_POINT) { missing C620 -- 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 21:18:08 +0000 Gerrit-HasComments: Yes

1 0